Re: [PATCH] scsi: qedf: Avoid reading past end of buffer

[Martin K. Petersen]

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

Re: [PATCH] scsi: qedf: Avoid reading past end of buffer

[Chad Dupuis]

On Fri, 5 May 2017, 7:10pm, Kees Cook wrote:

> On Fri, May 5, 2017 at 4:01 PM, Bart Van Assche
>  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, _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  

Re: [PATCH] scsi: qedf: Avoid reading past end of buffer

[Kees Cook]

On Fri, May 5, 2017 at 4:01 PM, Bart Van Assche
 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, _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

Re: [PATCH] scsi: qedf: Avoid reading past end of buffer

[Bart Van Assche]

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, _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.

[PATCH] scsi: qedf: Avoid reading past end of buffer

[Kees Cook]

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 
Signed-off-by: Kees Cook 
---
 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, _params);
if (rc) {
QEDF_ERR(&(qedf->dbg_ctx), "Cannot start slowpath.\n");
-- 
2.7.4


-- 
Kees Cook
Pixel Security