Bug 220094 - [scsi] sys/cam/scsi/scsi_sa.c: a sleep-under-mutex bug in saioctl
Summary: [scsi] sys/cam/scsi/scsi_sa.c: a sleep-under-mutex bug in saioctl
Status: Closed FIXED
Alias: None
Product: Base System
Classification: Unclassified
Component: kern (show other bugs)
Version: 11.0-RELEASE
Hardware: Any Any
: --- Affects Only Me
Assignee: Kenneth D. Merry
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2017-06-18 03:14 UTC by Jia-Ju Bai
Modified: 2017-06-27 14:12 UTC (History)
3 users (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Jia-Ju Bai 2017-06-18 03:14:42 UTC
The driver may sleep under a mutex, and the function call path in file "sys/cam/scsi/scsi_sa.c" in FreeBSD 11.0 is:
saioctl [line 1680: acquire the mutex]
  saextget [line 1683]
    malloc(M_WAITOK) [line 4444] --> may sleep

The possible fix of this bug is to replace "M_WAITOK" in malloc with "M_NOWAIT".

This bug is found by a static analysis tool written by myself, and it is checked by my review of the FreeBSD code.

Thanks,
Jia-Ju Bai
Comment 1 Kenneth D. Merry freebsd_committer freebsd_triage 2017-06-19 14:34:50 UTC
Thanks for the report, I'll take care of it.
Comment 2 commit-hook freebsd_committer freebsd_triage 2017-06-19 20:49:00 UTC
A commit references this bug:

Author: ken
Date: Mon Jun 19 20:48:01 UTC 2017
New revision: 320123
URL: https://svnweb.freebsd.org/changeset/base/320123

Log:
  Fix a potential sleep while holding a mutex in the sa(4) driver.

  If the user issues a MTIOCEXTGET ioctl, and the tape drive in question has
  a serial number that is longer than 80 characters, we malloc a buffer in
  saextget() to hold the output of cam_strvis().

  Since a mutex is held in that codepath, doing a M_WAITOK malloc could lead
  to sleeping while holding a mutex.  Change it to a M_NOWAIT malloc and bail
  out if we fail to allocate the memory.  Devices with serial numbers longer
  than 80 bytes are very rare (I don't recall seeing one), so this
  should be a very unusual case to hit.  But it is a bug that should be fixed.

  sys/cam/scsi/scsi_sa.c:
  	In saextget(), if we need to malloc a buffer to hold the output of
  	cam_strvis(), don't wait for the memory.  Fail and return an error
  	if we can't allocate the memory immediately.

  PR:		kern/220094
  Submitted by:	Jia-Ju Bai <baijiaju1990@163.com>
  MFC after:	3 days
  Sponsored by:	Spectra Logic

Changes:
  head/sys/cam/scsi/scsi_sa.c
Comment 3 commit-hook freebsd_committer freebsd_triage 2017-06-26 15:24:05 UTC
A commit references this bug:

Author: ken
Date: Mon Jun 26 15:23:12 UTC 2017
New revision: 320361
URL: https://svnweb.freebsd.org/changeset/base/320361

Log:
  MFC r320123:

    Fix a potential sleep while holding a mutex in the sa(4) driver.

    If the user issues a MTIOCEXTGET ioctl, and the tape drive in question has
    a serial number that is longer than 80 characters, we malloc a buffer in
    saextget() to hold the output of cam_strvis().

    Since a mutex is held in that codepath, doing a M_WAITOK malloc could lead
    to sleeping while holding a mutex.  Change it to a M_NOWAIT malloc and bail
    out if we fail to allocate the memory.  Devices with serial numbers longer
    than 80 bytes are very rare (I don't recall seeing one), so this
    should be a very unusual case to hit.  But it is a bug that should be fixed.

    sys/cam/scsi/scsi_sa.c:
    	In saextget(), if we need to malloc a buffer to hold the output of
    	cam_strvis(), don't wait for the memory.  Fail and return an error
    	if we can't allocate the memory immediately.

  PR:		kern/220094
  Submitted by:	Jia-Ju Bai <baijiaju1990@163.com>
  Sponsored by:	Spectra Logic

Changes:
_U  stable/10/
  stable/10/sys/cam/scsi/scsi_sa.c
Comment 4 commit-hook freebsd_committer freebsd_triage 2017-06-27 12:57:25 UTC
A commit references this bug:

Author: ken
Date: Tue Jun 27 12:56:37 UTC 2017
New revision: 320405
URL: https://svnweb.freebsd.org/changeset/base/320405

Log:
  MFC r320123:

    Fix a potential sleep while holding a mutex in the sa(4) driver.

    If the user issues a MTIOCEXTGET ioctl, and the tape drive in question has
    a serial number that is longer than 80 characters, we malloc a buffer in
    saextget() to hold the output of cam_strvis().

    Since a mutex is held in that codepath, doing a M_WAITOK malloc could lead
    to sleeping while holding a mutex.  Change it to a M_NOWAIT malloc and bail
    out if we fail to allocate the memory.  Devices with serial numbers longer
    than 80 bytes are very rare (I don't recall seeing one), so this
    should be a very unusual case to hit.  But it is a bug that should be fixed.

    sys/cam/scsi/scsi_sa.c:
    	In saextget(), if we need to malloc a buffer to hold the output of
    	cam_strvis(), don't wait for the memory.  Fail and return an error
    	if we can't allocate the memory immediately.

  PR:		kern/220094
  Submitted by:	Jia-Ju Bai <baijiaju1990@163.com>
  Sponsored by:	Spectra Logic
  Approved by:	re (gjb)

Changes:
_U  stable/11/
  stable/11/sys/cam/scsi/scsi_sa.c
Comment 5 Kenneth D. Merry freebsd_committer freebsd_triage 2017-06-27 14:12:59 UTC
Fixed in head, stable/11 and stable/10.  Thank you for the bug report!