scsi: qla2xxx: replace snprintf with strscpy
diff mbox series

Message ID 20190725054653.30729-1-xywang.sjtu@sjtu.edu.cn
State Deferred
Headers show
Series
  • scsi: qla2xxx: replace snprintf with strscpy
Related show

Commit Message

Wang Xiayang July 25, 2019, 5:46 a.m. UTC
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(-)

Comments

Himanshu Madhani Aug. 14, 2019, 2:08 p.m. UTC | #1
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>
Bart Van Assche Aug. 14, 2019, 3:25 p.m. UTC | #2
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.
Himanshu Madhani Aug. 14, 2019, 6:33 p.m. UTC | #3
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.

Patch
diff mbox series

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);