Bug 238014 - Potential NULL pointer dereference in function ata_dev_advinfo and nvme_dev_advinfo
Summary: Potential NULL pointer dereference in function ata_dev_advinfo and nvme_dev_a...
Status: Closed FIXED
Alias: None
Product: Base System
Classification: Unclassified
Component: kern (show other bugs)
Version: CURRENT
Hardware: Any Any
: --- Affects Many People
Assignee: Warner Losh
URL:
Keywords: patch
Depends on:
Blocks:
 
Reported: 2019-05-21 07:42 UTC by Young
Modified: 2021-07-21 16:19 UTC (History)
2 users (show)

See Also:


Attachments
Proposed patch (2.72 KB, application/mbox)
2019-05-21 07:42 UTC, Young
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Young 2019-05-21 07:42:31 UTC
Created attachment 204502 [details]
Proposed patch

There are two NULL pointer vulnerabilities in functions named ata_dev_advinfo (sys/cam/ata/ata_xpt.c) and nvme_dev_advinfo (sys/cam/nvme/nvme_xpt.c).

We take function ata_dev_advinfo for example.

                if (cdai->flags & CDAI_FLAG_STORE) {
                        if (device->physpath != NULL)
                                free(device->physpath, M_CAMXPT);
                        device->physpath_len = cdai->bufsiz;
                        /* Clear existing buffer if zero length */
                        if (cdai->bufsiz == 0)
                                break;
                        device->physpath = malloc(cdai->bufsiz, M_CAMXPT, M_NOWAIT);
                        if (device->physpath == NULL) {
                                start_ccb->ccb_h.status = CAM_REQ_ABORTED;
                                return;
                        }
                        memcpy(device->physpath, cdai->buf, cdai->bufsiz);


if the physical path is being stored and there is a malloc failure (malloc(9) is called with M_NOWAIT), we could wind up in a situation where the device's physpath_len is set to the length the user provided, but the physpath itself is NULL.

If another context then comes in to fetch the physical path value, we would wind up trying to memcpy a NULL pointer into the caller's buffer.

So, set the physpath_len to 0 when we free the physpath on entry into the store case for the physical path.  Reset the length to a non-zero value only after we've successfully malloced a buffer to hold it.

The attachment is the proposed patch.
Comment 1 commit-hook freebsd_committer 2021-07-13 20:13:58 UTC
A commit in branch main references this bug:

URL: https://cgit.FreeBSD.org/src/commit/?id=431ddd94360a9e86c91294eaa2c7b859911984b7

commit 431ddd94360a9e86c91294eaa2c7b859911984b7
Author:     Young Xiao <92siuyang@gmail.com>
AuthorDate: 2019-05-21 07:36:29 +0000
Commit:     Warner Losh <imp@FreeBSD.org>
CommitDate: 2021-07-13 20:13:21 +0000

    Fix potential NULL pointer dereference of device physical path

    In ata_dev_advinfo() and nvme_dev_advinfo(), if the physical path is
    being stored and there is a malloc failure (malloc(9) is called with
    M_NOWAIT), we could wind up in a situation where the device's
    physpath_len is set to the length the user provided, but the physpath
    itself is NULL.

    If another context then comes in to fetch the physical path value, we
    would wind up trying to memcpy a NULL pointer into the caller's buffer.

    So, set the physpath_len to 0 when we free the physpath on entry into
    the store case for the physical path.  Reset the length to a non-zero
    value only after we've successfully malloced a buffer to hold it.

    This code mirrors scsi_xpt.c does already as well.

    Signed-off-by:  Young Xiao <92siuyang@gmail.com>
    Reviewed by:    imp
    PR:             238014

 sys/cam/ata/ata_xpt.c   | 7 +++++--
 sys/cam/nvme/nvme_xpt.c | 7 +++++--
 2 files changed, 10 insertions(+), 4 deletions(-)
Comment 2 Warner Losh freebsd_committer 2021-07-13 20:18:06 UTC
Patch looks good. It's what scsi_xpt.c does already as well.
Committed and in my MFC queue, closing bug.
Comment 3 commit-hook freebsd_committer 2021-07-21 16:14:29 UTC
A commit in branch stable/13 references this bug:

URL: https://cgit.FreeBSD.org/src/commit/?id=c8b24162a4eb20484d4add7710a33ef6387111f7

commit c8b24162a4eb20484d4add7710a33ef6387111f7
Author:     Young Xiao <92siuyang@gmail.com>
AuthorDate: 2019-05-21 07:36:29 +0000
Commit:     Warner Losh <imp@FreeBSD.org>
CommitDate: 2021-07-21 16:13:10 +0000

    Fix potential NULL pointer dereference of device physical path

    In ata_dev_advinfo() and nvme_dev_advinfo(), if the physical path is
    being stored and there is a malloc failure (malloc(9) is called with
    M_NOWAIT), we could wind up in a situation where the device's
    physpath_len is set to the length the user provided, but the physpath
    itself is NULL.

    If another context then comes in to fetch the physical path value, we
    would wind up trying to memcpy a NULL pointer into the caller's buffer.

    So, set the physpath_len to 0 when we free the physpath on entry into
    the store case for the physical path.  Reset the length to a non-zero
    value only after we've successfully malloced a buffer to hold it.

    This code mirrors scsi_xpt.c does already as well.

    Signed-off-by:  Young Xiao <92siuyang@gmail.com>
    Reviewed by:    imp
    PR:             238014

    (cherry picked from commit 431ddd94360a9e86c91294eaa2c7b859911984b7)

 sys/cam/ata/ata_xpt.c   | 7 +++++--
 sys/cam/nvme/nvme_xpt.c | 7 +++++--
 2 files changed, 10 insertions(+), 4 deletions(-)
Comment 4 commit-hook freebsd_committer 2021-07-21 16:19:36 UTC
A commit in branch stable/12 references this bug:

URL: https://cgit.FreeBSD.org/src/commit/?id=2f5778abbe180dcb44eaf17db1fb915204249615

commit 2f5778abbe180dcb44eaf17db1fb915204249615
Author:     Young Xiao <92siuyang@gmail.com>
AuthorDate: 2019-05-21 07:36:29 +0000
Commit:     Warner Losh <imp@FreeBSD.org>
CommitDate: 2021-07-21 16:16:31 +0000

    Fix potential NULL pointer dereference of device physical path

    In ata_dev_advinfo() and nvme_dev_advinfo(), if the physical path is
    being stored and there is a malloc failure (malloc(9) is called with
    M_NOWAIT), we could wind up in a situation where the device's
    physpath_len is set to the length the user provided, but the physpath
    itself is NULL.

    If another context then comes in to fetch the physical path value, we
    would wind up trying to memcpy a NULL pointer into the caller's buffer.

    So, set the physpath_len to 0 when we free the physpath on entry into
    the store case for the physical path.  Reset the length to a non-zero
    value only after we've successfully malloced a buffer to hold it.

    This code mirrors scsi_xpt.c does already as well.

    Signed-off-by:  Young Xiao <92siuyang@gmail.com>
    Reviewed by:    imp
    PR:             238014

    (cherry picked from commit 431ddd94360a9e86c91294eaa2c7b859911984b7)

 sys/cam/ata/ata_xpt.c   | 7 +++++--
 sys/cam/nvme/nvme_xpt.c | 7 +++++--
 2 files changed, 10 insertions(+), 4 deletions(-)