Bug 238014
| Summary: | Potential NULL pointer dereference in function ata_dev_advinfo and nvme_dev_advinfo | ||||||
|---|---|---|---|---|---|---|---|
| Product: | Base System | Reporter: | Young <yangx92> | ||||
| Component: | kern | Assignee: | Warner Losh <imp> | ||||
| Status: | Closed FIXED | ||||||
| Severity: | Affects Many People | CC: | imp, splavecl | ||||
| Priority: | --- | Keywords: | patch | ||||
| Version: | CURRENT | ||||||
| Hardware: | Any | ||||||
| OS: | Any | ||||||
| Attachments: |
|
||||||
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(-) Patch looks good. It's what scsi_xpt.c does already as well. Committed and in my MFC queue, closing bug. 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(-) 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(-) |
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.