* Re: [PATCH] scsi: qla2xxx: replace snprintf with strscpy
[not found] <20190725054653.30729-1-xywang.sjtu@sjtu.edu.cn>
@ 2019-08-14 14:08 ` Himanshu Madhani
2019-08-14 15:25 ` Bart Van Assche
1 sibling, 0 replies; 3+ messages in thread
From: Himanshu Madhani @ 2019-08-14 14:08 UTC (permalink / raw)
To: Wang Xiayang; +Cc: linux-scsi
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>
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] scsi: qla2xxx: replace snprintf with strscpy
[not found] <20190725054653.30729-1-xywang.sjtu@sjtu.edu.cn>
2019-08-14 14:08 ` [PATCH] scsi: qla2xxx: replace snprintf with strscpy Himanshu Madhani
@ 2019-08-14 15:25 ` Bart Van Assche
2019-08-14 18:33 ` Himanshu Madhani
1 sibling, 1 reply; 3+ messages in thread
From: Bart Van Assche @ 2019-08-14 15:25 UTC (permalink / raw)
To: Wang Xiayang; +Cc: qla2xxx-upstream, linux-scsi
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.
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] scsi: qla2xxx: replace snprintf with strscpy
2019-08-14 15:25 ` Bart Van Assche
@ 2019-08-14 18:33 ` Himanshu Madhani
0 siblings, 0 replies; 3+ messages in thread
From: Himanshu Madhani @ 2019-08-14 18:33 UTC (permalink / raw)
To: Bart Van Assche, Wang Xiayang; +Cc: linux-scsi
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.
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2019-08-14 18:33 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <20190725054653.30729-1-xywang.sjtu@sjtu.edu.cn>
2019-08-14 14:08 ` [PATCH] scsi: qla2xxx: replace snprintf with strscpy Himanshu Madhani
2019-08-14 15:25 ` Bart Van Assche
2019-08-14 18:33 ` Himanshu Madhani
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.