Re: [PATCH] scsi: qla2xxx: replace snprintf with strscpy

Re: [PATCH] scsi: qla2xxx: replace snprintf with strscpy

[Himanshu Madhani]

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>

Re: [PATCH] scsi: qla2xxx: replace snprintf with strscpy

[Bart Van Assche]

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.

Re: [PATCH] scsi: qla2xxx: replace snprintf with strscpy

[Himanshu Madhani]

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.