* [PATCH] scsi: qedf: Avoid reading past end of buffer
@ 2017-05-05 22:42 Kees Cook
2017-05-05 23:01 ` Bart Van Assche
2017-05-09 2:08 ` Martin K. Petersen
0 siblings, 2 replies; 5+ messages in thread
From: Kees Cook @ 2017-05-05 22:42 UTC (permalink / raw)
To: linux-scsi
Cc: QLogic-Storage-Upstream, James E.J. Bottomley, Martin K. Petersen,
linux-kernel, Daniel Micay
Using memcpy() from a string that is shorter than the length copied means
the destination buffer is being filled with arbitrary data from the kernel
rodata segment. Instead, use strncpy() which will fill the trailing bytes
with zeros.
This was found with the future CONFIG_FORTIFY_SOURCE feature.
Cc: Daniel Micay <danielmicay@gmail.com>
Signed-off-by: Kees Cook <keescook@chromium.org>
---
drivers/scsi/qedf/qedf_main.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/scsi/qedf/qedf_main.c b/drivers/scsi/qedf/qedf_main.c
index cceddd995a4b..a5c97342fd5d 100644
--- a/drivers/scsi/qedf/qedf_main.c
+++ b/drivers/scsi/qedf/qedf_main.c
@@ -2895,7 +2895,7 @@ static int __qedf_probe(struct pci_dev *pdev, int mode)
slowpath_params.drv_minor = QEDF_DRIVER_MINOR_VER;
slowpath_params.drv_rev = QEDF_DRIVER_REV_VER;
slowpath_params.drv_eng = QEDF_DRIVER_ENG_VER;
- memcpy(slowpath_params.name, "qedf", QED_DRV_VER_STR_SIZE);
+ strncpy(slowpath_params.name, "qedf", QED_DRV_VER_STR_SIZE);
rc = qed_ops->common->slowpath_start(qedf->cdev, &slowpath_params);
if (rc) {
QEDF_ERR(&(qedf->dbg_ctx), "Cannot start slowpath.\n");
--
2.7.4
--
Kees Cook
Pixel Security
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: [PATCH] scsi: qedf: Avoid reading past end of buffer
2017-05-05 22:42 [PATCH] scsi: qedf: Avoid reading past end of buffer Kees Cook
@ 2017-05-05 23:01 ` Bart Van Assche
2017-05-05 23:10 ` Kees Cook
2017-05-09 2:08 ` Martin K. Petersen
1 sibling, 1 reply; 5+ messages in thread
From: Bart Van Assche @ 2017-05-05 23:01 UTC (permalink / raw)
To: keescook@chromium.org, linux-scsi@vger.kernel.org
Cc: jejb@linux.vnet.ibm.com, linux-kernel@vger.kernel.org,
QLogic-Storage-Upstream@cavium.com, danielmicay@gmail.com,
martin.petersen@oracle.com
On Fri, 2017-05-05 at 15:42 -0700, Kees Cook wrote:
> diff --git a/drivers/scsi/qedf/qedf_main.c b/drivers/scsi/qedf/qedf_main.c
> index cceddd995a4b..a5c97342fd5d 100644
> --- a/drivers/scsi/qedf/qedf_main.c
> +++ b/drivers/scsi/qedf/qedf_main.c
> @@ -2895,7 +2895,7 @@ static int __qedf_probe(struct pci_dev *pdev, int mode)
> slowpath_params.drv_minor = QEDF_DRIVER_MINOR_VER;
> slowpath_params.drv_rev = QEDF_DRIVER_REV_VER;
> slowpath_params.drv_eng = QEDF_DRIVER_ENG_VER;
> - memcpy(slowpath_params.name, "qedf", QED_DRV_VER_STR_SIZE);
> + strncpy(slowpath_params.name, "qedf", QED_DRV_VER_STR_SIZE);
> rc = qed_ops->common->slowpath_start(qedf->cdev, &slowpath_params);
> if (rc) {
> QEDF_ERR(&(qedf->dbg_ctx), "Cannot start slowpath.\n");
Hello Kees,
Although this patch looks fine to me, isn't strlcpy() preferred over strncpy()?
Bart.
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] scsi: qedf: Avoid reading past end of buffer
2017-05-05 23:01 ` Bart Van Assche
@ 2017-05-05 23:10 ` Kees Cook
2017-05-06 19:00 ` Chad Dupuis
0 siblings, 1 reply; 5+ messages in thread
From: Kees Cook @ 2017-05-05 23:10 UTC (permalink / raw)
To: Bart Van Assche
Cc: linux-scsi@vger.kernel.org, jejb@linux.vnet.ibm.com,
linux-kernel@vger.kernel.org, QLogic-Storage-Upstream@cavium.com,
danielmicay@gmail.com, martin.petersen@oracle.com
On Fri, May 5, 2017 at 4:01 PM, Bart Van Assche
<Bart.VanAssche@sandisk.com> wrote:
> On Fri, 2017-05-05 at 15:42 -0700, Kees Cook wrote:
>> diff --git a/drivers/scsi/qedf/qedf_main.c b/drivers/scsi/qedf/qedf_main.c
>> index cceddd995a4b..a5c97342fd5d 100644
>> --- a/drivers/scsi/qedf/qedf_main.c
>> +++ b/drivers/scsi/qedf/qedf_main.c
>> @@ -2895,7 +2895,7 @@ static int __qedf_probe(struct pci_dev *pdev, int mode)
>> slowpath_params.drv_minor = QEDF_DRIVER_MINOR_VER;
>> slowpath_params.drv_rev = QEDF_DRIVER_REV_VER;
>> slowpath_params.drv_eng = QEDF_DRIVER_ENG_VER;
>> - memcpy(slowpath_params.name, "qedf", QED_DRV_VER_STR_SIZE);
>> + strncpy(slowpath_params.name, "qedf", QED_DRV_VER_STR_SIZE);
>> rc = qed_ops->common->slowpath_start(qedf->cdev, &slowpath_params);
>> if (rc) {
>> QEDF_ERR(&(qedf->dbg_ctx), "Cannot start slowpath.\n");
>
> Hello Kees,
>
> Although this patch looks fine to me, isn't strlcpy() preferred over strncpy()?
strlcpy doesn't zero-pad, so I think strncpy is preferred here,
otherwise we may risk leaving portions of the destination buffer
filled with uninitialized data, maybe leaking kernel memory contents.
-Kees
--
Kees Cook
Pixel Security
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] scsi: qedf: Avoid reading past end of buffer
2017-05-05 23:10 ` Kees Cook
@ 2017-05-06 19:00 ` Chad Dupuis
0 siblings, 0 replies; 5+ messages in thread
From: Chad Dupuis @ 2017-05-06 19:00 UTC (permalink / raw)
To: Kees Cook
Cc: Bart Van Assche, linux-scsi@vger.kernel.org,
jejb@linux.vnet.ibm.com, linux-kernel@vger.kernel.org,
QLogic-Storage-Upstream@cavium.com, danielmicay@gmail.com,
martin.petersen@oracle.com
On Fri, 5 May 2017, 7:10pm, Kees Cook wrote:
> On Fri, May 5, 2017 at 4:01 PM, Bart Van Assche
> <Bart.VanAssche@sandisk.com> wrote:
> > On Fri, 2017-05-05 at 15:42 -0700, Kees Cook wrote:
> >> diff --git a/drivers/scsi/qedf/qedf_main.c b/drivers/scsi/qedf/qedf_main.c
> >> index cceddd995a4b..a5c97342fd5d 100644
> >> --- a/drivers/scsi/qedf/qedf_main.c
> >> +++ b/drivers/scsi/qedf/qedf_main.c
> >> @@ -2895,7 +2895,7 @@ static int __qedf_probe(struct pci_dev *pdev, int mode)
> >> slowpath_params.drv_minor = QEDF_DRIVER_MINOR_VER;
> >> slowpath_params.drv_rev = QEDF_DRIVER_REV_VER;
> >> slowpath_params.drv_eng = QEDF_DRIVER_ENG_VER;
> >> - memcpy(slowpath_params.name, "qedf", QED_DRV_VER_STR_SIZE);
> >> + strncpy(slowpath_params.name, "qedf", QED_DRV_VER_STR_SIZE);
> >> rc = qed_ops->common->slowpath_start(qedf->cdev, &slowpath_params);
> >> if (rc) {
> >> QEDF_ERR(&(qedf->dbg_ctx), "Cannot start slowpath.\n");
> >
> > Hello Kees,
> >
> > Although this patch looks fine to me, isn't strlcpy() preferred over strncpy()?
>
> strlcpy doesn't zero-pad, so I think strncpy is preferred here,
> otherwise we may risk leaving portions of the destination buffer
> filled with uninitialized data, maybe leaking kernel memory contents.
>
> -Kees
>
I'd agree with strncpy so we zero out the rest of the buffer.
Acked-by: Chad Dupuis <chad.dupuis@cavium.com>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] scsi: qedf: Avoid reading past end of buffer
2017-05-05 22:42 [PATCH] scsi: qedf: Avoid reading past end of buffer Kees Cook
2017-05-05 23:01 ` Bart Van Assche
@ 2017-05-09 2:08 ` Martin K. Petersen
1 sibling, 0 replies; 5+ messages in thread
From: Martin K. Petersen @ 2017-05-09 2:08 UTC (permalink / raw)
To: Kees Cook
Cc: linux-scsi, QLogic-Storage-Upstream, James E.J. Bottomley,
Martin K. Petersen, linux-kernel, Daniel Micay
Kees,
> Using memcpy() from a string that is shorter than the length copied
> means the destination buffer is being filled with arbitrary data from
> the kernel rodata segment. Instead, use strncpy() which will fill the
> trailing bytes with zeros.
Applied to 4.12/scsi-fixes, thanks!
--
Martin K. Petersen Oracle Linux Engineering
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2017-05-09 2:08 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-05-05 22:42 [PATCH] scsi: qedf: Avoid reading past end of buffer Kees Cook
2017-05-05 23:01 ` Bart Van Assche
2017-05-05 23:10 ` Kees Cook
2017-05-06 19:00 ` Chad Dupuis
2017-05-09 2:08 ` Martin K. Petersen
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox