[dpdk-stable] [PATH 17.11] net/nfp: fix misuse of strlcpy

[dpdk-stable] [PATH 17.11] net/nfp: fix misuse of strlcpy

[Alejandro Lucero]

Current strlcpy function is doing the wrong thing and as a consequence
the firmware does not find the symbol requested precluding the right
NFP initialization.

Using strncpy is safe here since the symbol length can never be longer
than the buffer size where the firmware will get the symbol to work
with. However, newer compilers do not allow to have the strncpy using
the source length as the third parameter, so this patch uses instead a
memcpy call with a previous memset for cleaning up the buffer to be
used by the firmware.

Fixes: a5d659c2d03f ("net/nfp: replace strncpy by strlcpy")

Signed-off-by: Alejandro Lucero <alejandro.lucero@netronome.com>
---
 drivers/net/nfp/nfp_nspu.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/net/nfp/nfp_nspu.c b/drivers/net/nfp/nfp_nspu.c
index ac5bce3b1..d4abb6c8e 100644
--- a/drivers/net/nfp/nfp_nspu.c
+++ b/drivers/net/nfp/nfp_nspu.c
@@ -424,7 +424,9 @@ nfp_nspu_set_bar_from_symbl(nspu_desc_t *desc, const char *symbl,
 	if (!sym_buf)
 		return -ENOMEM;
 
-	strlcpy(sym_buf, symbl, sizeof(sym_buf));
+	memset(sym_buf, 0, desc->buf_size);
+	memcpy(sym_buf, symbl, strlen(symbl));
+
 	ret = nspu_command(desc, NSP_CMD_GET_SYMBOL, 1, 1, sym_buf,
 			   NFP_SYM_DESC_LEN, strlen(symbl));
 	if (ret) {
-- 
2.17.1

Re: [dpdk-stable] [PATH 17.11] net/nfp: fix misuse of strlcpy

[Yongseok Koh]

> On Feb 15, 2019, at 7:30 PM, Alejandro Lucero <alejandro.lucero@netronome.com> wrote:
> 
> Current strlcpy function is doing the wrong thing and as a consequence
> the firmware does not find the symbol requested precluding the right
> NFP initialization.

Applied to stable/17.11.

However, can you explain what strlcpy does wrong?
If it is buggy, we should fix it as there are quite a few occurrences,
due to the new gcc.


Thanks,
Yongseok

> Using strncpy is safe here since the symbol length can never be longer
> than the buffer size where the firmware will get the symbol to work
> with. However, newer compilers do not allow to have the strncpy using
> the source length as the third parameter, so this patch uses instead a
> memcpy call with a previous memset for cleaning up the buffer to be
> used by the firmware.
> 
> Fixes: a5d659c2d03f ("net/nfp: replace strncpy by strlcpy")
> 
> Signed-off-by: Alejandro Lucero <alejandro.lucero@netronome.com>
> ---
> drivers/net/nfp/nfp_nspu.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/nfp/nfp_nspu.c b/drivers/net/nfp/nfp_nspu.c
> index ac5bce3b1..d4abb6c8e 100644
> --- a/drivers/net/nfp/nfp_nspu.c
> +++ b/drivers/net/nfp/nfp_nspu.c
> @@ -424,7 +424,9 @@ nfp_nspu_set_bar_from_symbl(nspu_desc_t *desc, const char *symbl,
> 	if (!sym_buf)
> 		return -ENOMEM;
> 
> -	strlcpy(sym_buf, symbl, sizeof(sym_buf));
> +	memset(sym_buf, 0, desc->buf_size);
> +	memcpy(sym_buf, symbl, strlen(symbl));
> +
> 	ret = nspu_command(desc, NSP_CMD_GET_SYMBOL, 1, 1, sym_buf,
> 			   NFP_SYM_DESC_LEN, strlen(symbl));
> 	if (ret) {
> -- 
> 2.17.1
> 

Re: [dpdk-stable] [PATH 17.11] net/nfp: fix misuse of strlcpy

[Alejandro Lucero]

On Sat, Feb 16, 2019 at 7:49 AM Yongseok Koh <yskoh@mellanox.com> wrote:

> > On Feb 15, 2019, at 7:30 PM, Alejandro Lucero <
> alejandro.lucero@netronome.com> wrote:
> >
> > Current strlcpy function is doing the wrong thing and as a consequence
> > the firmware does not find the symbol requested precluding the right
> > NFP initialization.
>
> Applied to stable/17.11.
>
>
Thanks!


> However, can you explain what strlcpy does wrong?
> If it is buggy, we should fix it as there are quite a few occurrences,
> due to the new gcc.
>
>
Not sure if this is a strlcpy bug. We were in a hurry for fixing this
before the release, so I had no time to investigate, just to get it fixed.
This code is not present in current PMD code, so I'm not keen to waste any
further time on this.

However, note the memset for zeroing the buffer before the copy. That was
also required sometimes with strncpy because the fw is really picky and
without the zeroing it failed with old fw. I tried to just add the zeroing
and keeping strlcpy, but it did not work. Although strlcpy ends up adding a
"\0", it does that after the length of the copied string, and the length
used in the strlcpy is not right. I would say string literals, used for the
char *sym, add a null character as well, so even which such a length based
on the destination buffer, it should work. So, it does not seem trivial why
the firwmare is not getting the symbol string as it requires.


>
> Thanks,
> Yongseok
>
> > Using strncpy is safe here since the symbol length can never be longer
> > than the buffer size where the firmware will get the symbol to work
> > with. However, newer compilers do not allow to have the strncpy using
> > the source length as the third parameter, so this patch uses instead a
> > memcpy call with a previous memset for cleaning up the buffer to be
> > used by the firmware.
> >
> > Fixes: a5d659c2d03f ("net/nfp: replace strncpy by strlcpy")
> >
> > Signed-off-by: Alejandro Lucero <alejandro.lucero@netronome.com>
> > ---
> > drivers/net/nfp/nfp_nspu.c | 4 +++-
> > 1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/net/nfp/nfp_nspu.c b/drivers/net/nfp/nfp_nspu.c
> > index ac5bce3b1..d4abb6c8e 100644
> > --- a/drivers/net/nfp/nfp_nspu.c
> > +++ b/drivers/net/nfp/nfp_nspu.c
> > @@ -424,7 +424,9 @@ nfp_nspu_set_bar_from_symbl(nspu_desc_t *desc, const
> char *symbl,
> >       if (!sym_buf)
> >               return -ENOMEM;
> >
> > -     strlcpy(sym_buf, symbl, sizeof(sym_buf));
> > +     memset(sym_buf, 0, desc->buf_size);
> > +     memcpy(sym_buf, symbl, strlen(symbl));
> > +
> >       ret = nspu_command(desc, NSP_CMD_GET_SYMBOL, 1, 1, sym_buf,
> >                          NFP_SYM_DESC_LEN, strlen(symbl));
> >       if (ret) {
> > --
> > 2.17.1
> >
>
>