[dpdk-dev,2/2] examples/vhost_scsi: fix potential buffer overrun with safe copy API
Checks
Commit Message
Signed-off-by: Changpeng Liu <changpeng.liu@intel.com>
---
examples/vhost_scsi/scsi.c | 23 ++++++++++++-----------
examples/vhost_scsi/vhost_scsi.c | 5 +++--
2 files changed, 15 insertions(+), 13 deletions(-)
Comments
18/05/2018 01:32, Changpeng Liu:
> Signed-off-by: Changpeng Liu <changpeng.liu@intel.com>
Missing explanations.
> - strlcpy((char *)vpage->params, bdev->name,
> - sizeof(vpage->params));
> + vhost_strcpy_pad((char *)vpage->params, bdev->name,
> + sizeof(vpage->params), ' ');
Why do you think vhost_strcpy_pad is safer than strlcpy?
> - strncpy(bdev->name, bdev_name, sizeof(bdev->name));
> - strncpy(bdev->product_name, bdev_serial, sizeof(bdev->product_name));
> + snprintf(bdev->name, sizeof(bdev->name), "%s", bdev_name);
> + snprintf(bdev->product_name, sizeof(bdev->product_name),
> + "%s", bdev_serial);
You should use strlcpy.
> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas@monjalon.net]
> Sent: Tuesday, May 22, 2018 10:48 AM
> To: Liu, Changpeng <changpeng.liu@intel.com>
> Cc: dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH 2/2] examples/vhost_scsi: fix potential buffer
> overrun with safe copy API
>
> 18/05/2018 01:32, Changpeng Liu:
> > Signed-off-by: Changpeng Liu <changpeng.liu@intel.com>
>
> Missing explanations.
>
> > - strlcpy((char *)vpage->params, bdev->name,
> > - sizeof(vpage->params));
> > + vhost_strcpy_pad((char *)vpage->params, bdev->name,
> > + sizeof(vpage->params), ' ');
>
> Why do you think vhost_strcpy_pad is safer than strlcpy?
A code Coverity issue 279452 reported for strlcpy, so here replace with internal API can avoid it.
>
> > - strncpy(bdev->name, bdev_name, sizeof(bdev->name));
> > - strncpy(bdev->product_name, bdev_serial, sizeof(bdev->product_name));
> > + snprintf(bdev->name, sizeof(bdev->name), "%s", bdev_name);
> > + snprintf(bdev->product_name, sizeof(bdev->product_name),
> > + "%s", bdev_serial);
>
> You should use strlcpy.
>
22/05/2018 19:58, Liu, Changpeng:
> From: Thomas Monjalon [mailto:thomas@monjalon.net]
> > 18/05/2018 01:32, Changpeng Liu:
> > > - strlcpy((char *)vpage->params, bdev->name,
> > > - sizeof(vpage->params));
> > > + vhost_strcpy_pad((char *)vpage->params, bdev->name,
> > > + sizeof(vpage->params), ' ');
> >
> > Why do you think vhost_strcpy_pad is safer than strlcpy?
>
> A code Coverity issue 279452 reported for strlcpy, so here replace with internal API can avoid it.
I think it is a false positive.
Remember that Coverity is just a tool.
@@ -182,8 +182,8 @@
break;
case SPC_VPD_UNIT_SERIAL_NUMBER:
hlen = 4;
- strlcpy((char *)vpage->params, bdev->name,
- sizeof(vpage->params));
+ vhost_strcpy_pad((char *)vpage->params, bdev->name,
+ sizeof(vpage->params), ' ');
vpage->alloc_len = rte_cpu_to_be_16(32);
break;
case SPC_VPD_DEVICE_IDENTIFICATION:
@@ -217,10 +217,11 @@
desig->piv = 1;
desig->reserved1 = 0;
desig->len = 8 + 16 + 32;
- strlcpy((char *)desig->desig, "INTEL", 8);
+ vhost_strcpy_pad((char *)desig->desig, "INTEL", 8, ' ');
vhost_strcpy_pad((char *)&desig->desig[8],
bdev->product_name, 16, ' ');
- strlcpy((char *)&desig->desig[24], bdev->name, 32);
+ vhost_strcpy_pad((char *)&desig->desig[24], bdev->name,
+ 32, ' ');
len += sizeof(struct scsi_desig_desc) + 8 + 16 + 32;
buf += sizeof(struct scsi_desig_desc) + desig->len;
@@ -277,17 +278,17 @@
inqdata->flags3 = 0x2;
/* T10 VENDOR IDENTIFICATION */
- strlcpy((char *)inqdata->t10_vendor_id, "INTEL",
- sizeof(inqdata->t10_vendor_id));
+ vhost_strcpy_pad((char *)inqdata->t10_vendor_id, "INTEL",
+ sizeof(inqdata->t10_vendor_id), ' ');
/* PRODUCT IDENTIFICATION */
- snprintf((char *)inqdata->product_id,
- RTE_DIM(inqdata->product_id), "%s",
- bdev->product_name);
+ vhost_strcpy_pad((char *)inqdata->product_id,
+ bdev->product_name,
+ sizeof(inqdata->product_id), ' ');
/* PRODUCT REVISION LEVEL */
- strlcpy((char *)inqdata->product_rev, "0001",
- sizeof(inqdata->product_rev));
+ vhost_strcpy_pad((char *)inqdata->product_rev, "0001",
+ sizeof(inqdata->product_rev), ' ');
/* Standard inquiry data ends here. Only populate
* remaining fields if alloc_len indicates enough
@@ -183,8 +183,9 @@ static uint64_t gpa_to_vva(int vid, uint64_t gpa, uint64_t *len)
if (!bdev)
return NULL;
- strncpy(bdev->name, bdev_name, sizeof(bdev->name));
- strncpy(bdev->product_name, bdev_serial, sizeof(bdev->product_name));
+ snprintf(bdev->name, sizeof(bdev->name), "%s", bdev_name);
+ snprintf(bdev->product_name, sizeof(bdev->product_name),
+ "%s", bdev_serial);
bdev->blocklen = blk_size;
bdev->blockcnt = blk_cnt;
bdev->write_cache = wce_enable;