[PATCH] staging: rtl8712: Replace snprintf with scnprintf

[PATCH] staging: rtl8712: Replace snprintf with scnprintf

[Rohit Sarkar]

When the number of bytes to be printed exceeds the limit snprintf
returns the number of bytes that would have been printed (if there was
no truncation). This might cause issues, hence use scnprintf which
returns the actual number of bytes printed to buffer always.

Signed-off-by: Rohit Sarkar <rohitsarkar5398@gmail.com>
---
 drivers/staging/rtl8712/rtl871x_ioctl_linux.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/rtl8712/rtl871x_ioctl_linux.c b/drivers/staging/rtl8712/rtl871x_ioctl_linux.c
index b08b9a191a34..ff5edcaba64d 100644
--- a/drivers/staging/rtl8712/rtl871x_ioctl_linux.c
+++ b/drivers/staging/rtl8712/rtl871x_ioctl_linux.c
@@ -142,7 +142,7 @@ static noinline_for_stack char *translate_scan_wpa(struct iw_request_info *info,
 		memset(buf, 0, MAX_WPA_IE_LEN);
 		n = sprintf(buf, "wpa_ie=");
 		for (i = 0; i < wpa_len; i++) {
-			n += snprintf(buf + n, MAX_WPA_IE_LEN - n,
+			n += scnprintf(buf + n, MAX_WPA_IE_LEN - n,
 						"%02x", wpa_ie[i]);
 			if (n >= MAX_WPA_IE_LEN)
 				break;
@@ -162,7 +162,7 @@ static noinline_for_stack char *translate_scan_wpa(struct iw_request_info *info,
 		memset(buf, 0, MAX_WPA_IE_LEN);
 		n = sprintf(buf, "rsn_ie=");
 		for (i = 0; i < rsn_len; i++) {
-			n += snprintf(buf + n, MAX_WPA_IE_LEN - n,
+			n += scnprintf(buf + n, MAX_WPA_IE_LEN - n,
 						"%02x", rsn_ie[i]);
 			if (n >= MAX_WPA_IE_LEN)
 				break;
-- 
2.17.1

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] staging: rtl8712: Replace snprintf with scnprintf

[Rohit Sarkar]

Resending as I made a typo in Larry's email id.

On Wed, Sep 11, 2019 at 12:19:31AM +0530, Rohit Sarkar wrote:
> When the number of bytes to be printed exceeds the limit snprintf
> returns the number of bytes that would have been printed (if there was
> no truncation). This might cause issues, hence use scnprintf which
> returns the actual number of bytes printed to buffer always.
> 
> Signed-off-by: Rohit Sarkar <rohitsarkar5398@gmail.com>
> ---
>  drivers/staging/rtl8712/rtl871x_ioctl_linux.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/staging/rtl8712/rtl871x_ioctl_linux.c b/drivers/staging/rtl8712/rtl871x_ioctl_linux.c
> index b08b9a191a34..ff5edcaba64d 100644
> --- a/drivers/staging/rtl8712/rtl871x_ioctl_linux.c
> +++ b/drivers/staging/rtl8712/rtl871x_ioctl_linux.c
> @@ -142,7 +142,7 @@ static noinline_for_stack char *translate_scan_wpa(struct iw_request_info *info,
>  		memset(buf, 0, MAX_WPA_IE_LEN);
>  		n = sprintf(buf, "wpa_ie=");
>  		for (i = 0; i < wpa_len; i++) {
> -			n += snprintf(buf + n, MAX_WPA_IE_LEN - n,
> +			n += scnprintf(buf + n, MAX_WPA_IE_LEN - n,
>  						"%02x", wpa_ie[i]);
>  			if (n >= MAX_WPA_IE_LEN)
>  				break;
> @@ -162,7 +162,7 @@ static noinline_for_stack char *translate_scan_wpa(struct iw_request_info *info,
>  		memset(buf, 0, MAX_WPA_IE_LEN);
>  		n = sprintf(buf, "rsn_ie=");
>  		for (i = 0; i < rsn_len; i++) {
> -			n += snprintf(buf + n, MAX_WPA_IE_LEN - n,
> +			n += scnprintf(buf + n, MAX_WPA_IE_LEN - n,
>  						"%02x", rsn_ie[i]);
>  			if (n >= MAX_WPA_IE_LEN)
>  				break;
> -- 
> 2.17.1
> 
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] staging: rtl8712: Replace snprintf with scnprintf

[Dan Carpenter]

On Wed, Sep 11, 2019 at 12:25:03AM +0530, Rohit Sarkar wrote:
> Resending as I made a typo in Larry's email id.
> 

I commented on the earlier email, but this email doesn't apply with
`git am` so it would be difficult for Larry to review it.

regards,
dan carpenter

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] staging: rtl8712: Replace snprintf with scnprintf

[Dan Carpenter]

On Wed, Sep 11, 2019 at 12:19:31AM +0530, Rohit Sarkar wrote:
> When the number of bytes to be printed exceeds the limit snprintf
> returns the number of bytes that would have been printed (if there was
> no truncation). This might cause issues, hence use scnprintf which
                  ^^^^^^^^^^^^^^^^^^^^^^^
Nope.  The code handles that situation fine so it will not cause issues.

> returns the actual number of bytes printed to buffer always.
> 
> Signed-off-by: Rohit Sarkar <rohitsarkar5398@gmail.com>
> ---
>  drivers/staging/rtl8712/rtl871x_ioctl_linux.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/staging/rtl8712/rtl871x_ioctl_linux.c b/drivers/staging/rtl8712/rtl871x_ioctl_linux.c
> index b08b9a191a34..ff5edcaba64d 100644
> --- a/drivers/staging/rtl8712/rtl871x_ioctl_linux.c
> +++ b/drivers/staging/rtl8712/rtl871x_ioctl_linux.c
> @@ -142,7 +142,7 @@ static noinline_for_stack char *translate_scan_wpa(struct iw_request_info *info,
>  		memset(buf, 0, MAX_WPA_IE_LEN);
>  		n = sprintf(buf, "wpa_ie=");
>  		for (i = 0; i < wpa_len; i++) {
> -			n += snprintf(buf + n, MAX_WPA_IE_LEN - n,
> +			n += scnprintf(buf + n, MAX_WPA_IE_LEN - n,
>  						"%02x", wpa_ie[i]);
>  			if (n >= MAX_WPA_IE_LEN)
                            ^^^^^^^^^^^^^^^^^^^
It checks for overflow here.  This check is impossible now and doesn't
make sense.  The other loop is similar.

>  				break;

regards,
dan carpenter

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] staging: rtl8712: Replace snprintf with scnprintf

[Rohit Sarkar]

On Tue, Oct 01, 2019 at 11:45:14AM +0300, Dan Carpenter wrote:
> > diff --git a/drivers/staging/rtl8712/rtl871x_ioctl_linux.c b/drivers/staging/rtl8712/rtl871x_ioctl_linux.c
> > index b08b9a191a34..ff5edcaba64d 100644
> > --- a/drivers/staging/rtl8712/rtl871x_ioctl_linux.c
> > +++ b/drivers/staging/rtl8712/rtl871x_ioctl_linux.c
> > @@ -142,7 +142,7 @@ static noinline_for_stack char *translate_scan_wpa(struct iw_request_info *info,
> >  		memset(buf, 0, MAX_WPA_IE_LEN);
> >  		n = sprintf(buf, "wpa_ie=");
> >  		for (i = 0; i < wpa_len; i++) {
> > -			n += snprintf(buf + n, MAX_WPA_IE_LEN - n,
> > +			n += scnprintf(buf + n, MAX_WPA_IE_LEN - n,
> >  						"%02x", wpa_ie[i]);
> >  			if (n >= MAX_WPA_IE_LEN)
>                             ^^^^^^^^^^^^^^^^^^^
> It checks for overflow here.  This check is impossible now and doesn't
> make sense.  The other loop is similar.

Good catch! I must have overlooked this.
"n" cannot be greater than MAX_WPA_IE_LEN but it can be equal to that
value. We can replace the '>=' with '==' so that we don't loop
unnecessarily when n has reached it's threshold.
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] staging: rtl8712: Replace snprintf with scnprintf

[Dan Carpenter]

On Tue, Oct 01, 2019 at 11:09:26PM +0530, Rohit Sarkar wrote:
> On Tue, Oct 01, 2019 at 11:45:14AM +0300, Dan Carpenter wrote:
> > > diff --git a/drivers/staging/rtl8712/rtl871x_ioctl_linux.c b/drivers/staging/rtl8712/rtl871x_ioctl_linux.c
> > > index b08b9a191a34..ff5edcaba64d 100644
> > > --- a/drivers/staging/rtl8712/rtl871x_ioctl_linux.c
> > > +++ b/drivers/staging/rtl8712/rtl871x_ioctl_linux.c
> > > @@ -142,7 +142,7 @@ static noinline_for_stack char *translate_scan_wpa(struct iw_request_info *info,
> > >  		memset(buf, 0, MAX_WPA_IE_LEN);
> > >  		n = sprintf(buf, "wpa_ie=");
> > >  		for (i = 0; i < wpa_len; i++) {
> > > -			n += snprintf(buf + n, MAX_WPA_IE_LEN - n,
> > > +			n += scnprintf(buf + n, MAX_WPA_IE_LEN - n,
> > >  						"%02x", wpa_ie[i]);
> > >  			if (n >= MAX_WPA_IE_LEN)
> >                             ^^^^^^^^^^^^^^^^^^^
> > It checks for overflow here.  This check is impossible now and doesn't
> > make sense.  The other loop is similar.
> 
> Good catch! I must have overlooked this.
> "n" cannot be greater than MAX_WPA_IE_LEN but it can be equal to that
> value. We can replace the '>=' with '==' so that we don't loop
> unnecessarily when n has reached it's threshold.

No.  scnprintf() returns the number of characters *not counting the
NUL terminator*.  So it can be a maximum of MAX_WPA_IE_LEN - 1.

regards,
dan carpenter

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] staging: rtl8712: Replace snprintf with scnprintf

[Rohit Sarkar]

On Tue, Oct 01, 2019 at 10:00:56PM +0300, Dan Carpenter wrote:
> 
> No.  scnprintf() returns the number of characters *not counting the
> NUL terminator*.  So it can be a maximum of MAX_WPA_IE_LEN - 1.
> 
> regards,
> dan carpenter

TIL :)
Would the better approach be to just remove the loop or break when n ==
MAX_WPA_IE_LEN - 1.

regards,
rohit
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] staging: rtl8712: Replace snprintf with scnprintf

[Dan Carpenter]

On Wed, Oct 02, 2019 at 10:03:51AM +0530, Rohit Sarkar wrote:
> On Tue, Oct 01, 2019 at 10:00:56PM +0300, Dan Carpenter wrote:
> > 
> > No.  scnprintf() returns the number of characters *not counting the
> > NUL terminator*.  So it can be a maximum of MAX_WPA_IE_LEN - 1.
> > 
> > regards,
> > dan carpenter
> 
> TIL :)
> Would the better approach be to just remove the loop or break when n ==
> MAX_WPA_IE_LEN - 1.

We could leave it as is or change it to "MAX_WPA_IE_LEN - 1".  But I
feel like the default should be to leave it as is unless there is a good
reason.

So from a static analysis perspective we wouldn't complain unless/until
the "n" is re-used outside the loop.  So this a chance to make a smarter
static analyzer.

regards,
dan carpenter

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] staging: rtl8712: Replace snprintf with scnprintf

[Rohit Sarkar]

On Wed, Oct 02, 2019 at 01:57:22PM +0300, Dan Carpenter wrote:
> 
> We could leave it as is or change it to "MAX_WPA_IE_LEN - 1".  But I
> feel like the default should be to leave it as is unless there is a good
> reason.

Makes sense, although greg has already merged this. I guess I will
remove the redundant check then.

thanks,
rohit
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] staging: rtl8712: Replace snprintf with scnprintf

[Dan Carpenter]

On Wed, Oct 02, 2019 at 05:12:14PM +0530, Rohit Sarkar wrote:
> On Wed, Oct 02, 2019 at 01:57:22PM +0300, Dan Carpenter wrote:
> > 
> > We could leave it as is or change it to "MAX_WPA_IE_LEN - 1".  But I
> > feel like the default should be to leave it as is unless there is a good
> > reason.
> 
> Makes sense, although greg has already merged this. I guess I will
> remove the redundant check then.

You could remove it, but I feel like it's better to check for
"== MAX_WPA_IE_LEN - 1".  They're effectively the same, but to me it
feels cleaner to be explicit how we're handling truncated data.

regards,
dan carpenter

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] staging: rtl8712: Replace snprintf with scnprintf

[Rohit Sarkar]

On Wed, Oct 02, 2019 at 03:06:22PM +0300, Dan Carpenter wrote:
> You could remove it, but I feel like it's better to check for
> "== MAX_WPA_IE_LEN - 1".  They're effectively the same, but to me it
> feels cleaner to be explicit how we're handling truncated data.
> 
> regards,
> dan carpenter

I feel the same way. Plus we would also avoid looping unnecessarily if
"n" exceeds the threshold. Will send a patch asap.

regards,
rohit
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel