Message ID | 20190725054653.30729-1-xywang.sjtu@sjtu.edu.cn (mailing list archive) |
---|---|
State | Deferred |
Headers | show |
Series | scsi: qla2xxx: replace snprintf with strscpy | expand |
On 7/25/19, 12:54 AM, "linux-scsi-owner@vger.kernel.org on behalf of Wang Xiayang" <linux-scsi-owner@vger.kernel.org on behalf of xywang.sjtu@sjtu.edu.cn> wrote: As commit a86028f8e3ee ("staging: most: sound: replace snprintf with strscpy") suggested, using snprintf without a format specifier is potentially risky if a0->vendor_name or a0->vendor_pn mistakenly contain format specifiers. In addition, as compared in the implementation, strscpy looks more light-weight than snprintf. This patch does not incur any functional change. Signed-off-by: Wang Xiayang <xywang.sjtu@sjtu.edu.cn> --- drivers/scsi/qla2xxx/qla_init.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/scsi/qla2xxx/qla_init.c b/drivers/scsi/qla2xxx/qla_init.c index 4059655639d9..068b54218ff4 100644 --- a/drivers/scsi/qla2xxx/qla_init.c +++ b/drivers/scsi/qla2xxx/qla_init.c @@ -3461,12 +3461,12 @@ static void qla2xxx_print_sfp_info(struct scsi_qla_host *vha) int leftover, len; memset(str, 0, STR_LEN); - snprintf(str, SFF_VEN_NAME_LEN+1, a0->vendor_name); + strscpy(str, a0->vendor_name, SFF_VEN_NAME_LEN+1); ql_dbg(ql_dbg_init, vha, 0x015a, "SFP MFG Name: %s\n", str); memset(str, 0, STR_LEN); - snprintf(str, SFF_PART_NAME_LEN+1, a0->vendor_pn); + strscpy(str, a0->vendor_pn, SFF_PART_NAME_LEN+1); ql_dbg(ql_dbg_init, vha, 0x015c, "SFP Part Name: %s\n", str); -- 2.11.0 Looks Good. Acked-by: Himanshu Madhani <hmadhani@marvell.com>
On 7/24/19 10:46 PM, Wang Xiayang wrote: > As commit a86028f8e3ee ("staging: most: sound: replace snprintf > with strscpy") suggested, using snprintf without a format specifier > is potentially risky if a0->vendor_name or a0->vendor_pn mistakenly > contain format specifiers. In addition, as compared in the > implementation, strscpy looks more light-weight than snprintf. > > This patch does not incur any functional change. > > Signed-off-by: Wang Xiayang <xywang.sjtu@sjtu.edu.cn> > --- > drivers/scsi/qla2xxx/qla_init.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/scsi/qla2xxx/qla_init.c b/drivers/scsi/qla2xxx/qla_init.c > index 4059655639d9..068b54218ff4 100644 > --- a/drivers/scsi/qla2xxx/qla_init.c > +++ b/drivers/scsi/qla2xxx/qla_init.c > @@ -3461,12 +3461,12 @@ static void qla2xxx_print_sfp_info(struct scsi_qla_host *vha) > int leftover, len; > > memset(str, 0, STR_LEN); > - snprintf(str, SFF_VEN_NAME_LEN+1, a0->vendor_name); > + strscpy(str, a0->vendor_name, SFF_VEN_NAME_LEN+1); > ql_dbg(ql_dbg_init, vha, 0x015a, > "SFP MFG Name: %s\n", str); > > memset(str, 0, STR_LEN); > - snprintf(str, SFF_PART_NAME_LEN+1, a0->vendor_pn); > + strscpy(str, a0->vendor_pn, SFF_PART_NAME_LEN+1); > ql_dbg(ql_dbg_init, vha, 0x015c, > "SFP Part Name: %s\n", str); From qla_def.h: /* Refer to SNIA SFF 8247 */ struct sff_8247_a0 { [ ... ] u8 vendor_name[SFF_VEN_NAME_LEN]; /* offset 20/14h */ u8 vendor_pn[SFF_PART_NAME_LEN]; /* part number */ So I think that using SFF_PART_NAME_LEN+1 as length limit is wrong. Himanshu, do you perhaps know whether or not the vendor_name and vendor_pn arrays should be '\0'-terminated in struct sff_8247_a0? Thanks, Bart.
On 8/14/19, 10:25 AM, "linux-scsi-owner@vger.kernel.org on behalf of Bart Van Assche" <linux-scsi-owner@vger.kernel.org on behalf of bvanassche@acm.org> wrote: On 7/24/19 10:46 PM, Wang Xiayang wrote: > As commit a86028f8e3ee ("staging: most: sound: replace snprintf > with strscpy") suggested, using snprintf without a format specifier > is potentially risky if a0->vendor_name or a0->vendor_pn mistakenly > contain format specifiers. In addition, as compared in the > implementation, strscpy looks more light-weight than snprintf. > > This patch does not incur any functional change. > > Signed-off-by: Wang Xiayang <xywang.sjtu@sjtu.edu.cn> > --- > drivers/scsi/qla2xxx/qla_init.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/scsi/qla2xxx/qla_init.c b/drivers/scsi/qla2xxx/qla_init.c > index 4059655639d9..068b54218ff4 100644 > --- a/drivers/scsi/qla2xxx/qla_init.c > +++ b/drivers/scsi/qla2xxx/qla_init.c > @@ -3461,12 +3461,12 @@ static void qla2xxx_print_sfp_info(struct scsi_qla_host *vha) > int leftover, len; > > memset(str, 0, STR_LEN); > - snprintf(str, SFF_VEN_NAME_LEN+1, a0->vendor_name); > + strscpy(str, a0->vendor_name, SFF_VEN_NAME_LEN+1); > ql_dbg(ql_dbg_init, vha, 0x015a, > "SFP MFG Name: %s\n", str); > > memset(str, 0, STR_LEN); > - snprintf(str, SFF_PART_NAME_LEN+1, a0->vendor_pn); > + strscpy(str, a0->vendor_pn, SFF_PART_NAME_LEN+1); > ql_dbg(ql_dbg_init, vha, 0x015c, > "SFP Part Name: %s\n", str); From qla_def.h: /* Refer to SNIA SFF 8247 */ struct sff_8247_a0 { [ ... ] u8 vendor_name[SFF_VEN_NAME_LEN]; /* offset 20/14h */ u8 vendor_pn[SFF_PART_NAME_LEN]; /* part number */ So I think that using SFF_PART_NAME_LEN+1 as length limit is wrong. Himanshu, do you perhaps know whether or not the vendor_name and vendor_pn arrays should be '\0'-terminated in struct sff_8247_a0? Hi Bart, Since the data is coming from firmware itself so it's not \0 terminated. So yes the array should be terminated with \0. Thanks, Himanshu Thanks, Bart.
diff --git a/drivers/scsi/qla2xxx/qla_init.c b/drivers/scsi/qla2xxx/qla_init.c index 4059655639d9..068b54218ff4 100644 --- a/drivers/scsi/qla2xxx/qla_init.c +++ b/drivers/scsi/qla2xxx/qla_init.c @@ -3461,12 +3461,12 @@ static void qla2xxx_print_sfp_info(struct scsi_qla_host *vha) int leftover, len; memset(str, 0, STR_LEN); - snprintf(str, SFF_VEN_NAME_LEN+1, a0->vendor_name); + strscpy(str, a0->vendor_name, SFF_VEN_NAME_LEN+1); ql_dbg(ql_dbg_init, vha, 0x015a, "SFP MFG Name: %s\n", str); memset(str, 0, STR_LEN); - snprintf(str, SFF_PART_NAME_LEN+1, a0->vendor_pn); + strscpy(str, a0->vendor_pn, SFF_PART_NAME_LEN+1); ql_dbg(ql_dbg_init, vha, 0x015c, "SFP Part Name: %s\n", str);
As commit a86028f8e3ee ("staging: most: sound: replace snprintf with strscpy") suggested, using snprintf without a format specifier is potentially risky if a0->vendor_name or a0->vendor_pn mistakenly contain format specifiers. In addition, as compared in the implementation, strscpy looks more light-weight than snprintf. This patch does not incur any functional change. Signed-off-by: Wang Xiayang <xywang.sjtu@sjtu.edu.cn> --- drivers/scsi/qla2xxx/qla_init.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)