[PATCH] iio: gyro: adis16136: use scnprintf instead of snprintf

[PATCH] iio: gyro: adis16136: use scnprintf instead of snprintf

[Rohit Sarkar]

scnprintf returns the actual number of bytes written into the buffer as
opposed to snprintf which returns the number of bytes that would have
been written if the buffer was big enough. Using the output of snprintf
may lead to difficult to detect bugs.

Thanks,
Rohit

Signed-off-by: Rohit Sarkar <rohitsarkar5398@gmail.com>
---
 drivers/iio/gyro/adis16136.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/iio/gyro/adis16136.c b/drivers/iio/gyro/adis16136.c
index a4c967a5fc5c..0a8bb02dc4b9 100644
--- a/drivers/iio/gyro/adis16136.c
+++ b/drivers/iio/gyro/adis16136.c
@@ -96,7 +96,7 @@ static ssize_t adis16136_show_serial(struct file *file,
 	if (ret)
 		return ret;
 
-	len = snprintf(buf, sizeof(buf), "%.4x%.4x%.4x-%.4x\n", lot1, lot2,
+	len = scnprintf(buf, sizeof(buf), "%.4x%.4x%.4x-%.4x\n", lot1, lot2,
 		lot3, serial);
 
 	return simple_read_from_buffer(userbuf, count, ppos, buf, len);
-- 
2.23.0.385.gbc12974a89

Re: [PATCH] iio: gyro: adis16136: use scnprintf instead of snprintf

[Andy Shevchenko]

On Wed, Mar 18, 2020 at 08:25:22PM +0530, Rohit Sarkar wrote:
> scnprintf returns the actual number of bytes written into the buffer as
> opposed to snprintf which returns the number of bytes that would have
> been written if the buffer was big enough. Using the output of snprintf
> may lead to difficult to detect bugs.

Nice. Have you investigate the code?

> @@ -96,7 +96,7 @@ static ssize_t adis16136_show_serial(struct file *file,
>  	if (ret)
>  		return ret;
>  
> -	len = snprintf(buf, sizeof(buf), "%.4x%.4x%.4x-%.4x\n", lot1, lot2,
> +	len = scnprintf(buf, sizeof(buf), "%.4x%.4x%.4x-%.4x\n", lot1, lot2,
>  		lot3, serial);
>  
>  	return simple_read_from_buffer(userbuf, count, ppos, buf, len);

The buffer size is 20, the pattern size I count to 19. Do you think snprintf()
can fail?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] iio: gyro: adis16136: use scnprintf instead of snprintf

[Rohit Sarkar]

On Sun, Mar 22, 2020 at 02:25:42AM +0200, Andy Shevchenko wrote:
> On Wed, Mar 18, 2020 at 08:25:22PM +0530, Rohit Sarkar wrote:
> > scnprintf returns the actual number of bytes written into the buffer as
> > opposed to snprintf which returns the number of bytes that would have
> > been written if the buffer was big enough. Using the output of snprintf
> > may lead to difficult to detect bugs.
> 
> Nice. Have you investigate the code?
> 
> > @@ -96,7 +96,7 @@ static ssize_t adis16136_show_serial(struct file *file,
> >  	if (ret)
> >  		return ret;
> >  
> > -	len = snprintf(buf, sizeof(buf), "%.4x%.4x%.4x-%.4x\n", lot1, lot2,
> > +	len = scnprintf(buf, sizeof(buf), "%.4x%.4x%.4x-%.4x\n", lot1, lot2,
> >  		lot3, serial);
> >  
> >  	return simple_read_from_buffer(userbuf, count, ppos, buf, len);
> 
> The buffer size is 20, the pattern size I count to 19. Do you think snprintf()
> can fail?
That might be the case, but IMO using scnprintf can be considered as a
best practice. There is no overhead with this change and further if the
pattern is changed by someone in the future they might overlook the
buffersize
Thanks,
Rohit
> -- 
> With Best Regards,
> Andy Shevchenko
> 
> 

Re: [PATCH] iio: gyro: adis16136: use scnprintf instead of snprintf

[Andy Shevchenko]

On Sun, Mar 22, 2020 at 8:11 AM Rohit Sarkar <rohitsarkar5398@gmail.com> wrote:
>
> On Sun, Mar 22, 2020 at 02:25:42AM +0200, Andy Shevchenko wrote:
> > On Wed, Mar 18, 2020 at 08:25:22PM +0530, Rohit Sarkar wrote:
> > > scnprintf returns the actual number of bytes written into the buffer as
> > > opposed to snprintf which returns the number of bytes that would have
> > > been written if the buffer was big enough. Using the output of snprintf
> > > may lead to difficult to detect bugs.
> >
> > Nice. Have you investigate the code?
> >
> > > @@ -96,7 +96,7 @@ static ssize_t adis16136_show_serial(struct file *file,
> > >     if (ret)
> > >             return ret;
> > >
> > > -   len = snprintf(buf, sizeof(buf), "%.4x%.4x%.4x-%.4x\n", lot1, lot2,
> > > +   len = scnprintf(buf, sizeof(buf), "%.4x%.4x%.4x-%.4x\n", lot1, lot2,
> > >             lot3, serial);
> > >
> > >     return simple_read_from_buffer(userbuf, count, ppos, buf, len);
> >
> > The buffer size is 20, the pattern size I count to 19. Do you think snprintf()
> > can fail?
> That might be the case, but IMO using scnprintf can be considered as a
> best practice. There is no overhead with this change and further if the
> pattern is changed by someone in the future they might overlook the
> buffersize

If we cut the string above we will give wrong information to the user space.
I think scnprintf() change is a noise and does not improve the situation anyhow.

So, when anybody modifying such code the test should be performed.


-- 
With Best Regards,
Andy Shevchenko

RE: [PATCH] iio: gyro: adis16136: use scnprintf instead of snprintf

[David Laight]

From: Andy Shevchenko
> Sent: 22 March 2020 10:27
> On Sun, Mar 22, 2020 at 8:11 AM Rohit Sarkar <rohitsarkar5398@gmail.com> wrote:
> >
> > On Sun, Mar 22, 2020 at 02:25:42AM +0200, Andy Shevchenko wrote:
> > > On Wed, Mar 18, 2020 at 08:25:22PM +0530, Rohit Sarkar wrote:
> > > > scnprintf returns the actual number of bytes written into the buffer as
> > > > opposed to snprintf which returns the number of bytes that would have
> > > > been written if the buffer was big enough. Using the output of snprintf
> > > > may lead to difficult to detect bugs.
> > >
> > > Nice. Have you investigate the code?
> > >
> > > > @@ -96,7 +96,7 @@ static ssize_t adis16136_show_serial(struct file *file,
> > > >     if (ret)
> > > >             return ret;
> > > >
> > > > -   len = snprintf(buf, sizeof(buf), "%.4x%.4x%.4x-%.4x\n", lot1, lot2,
> > > > +   len = scnprintf(buf, sizeof(buf), "%.4x%.4x%.4x-%.4x\n", lot1, lot2,
> > > >             lot3, serial);
> > > >
> > > >     return simple_read_from_buffer(userbuf, count, ppos, buf, len);
> > >
> > > The buffer size is 20, the pattern size I count to 19. Do you think snprintf()
> > > can fail?
> > That might be the case, but IMO using scnprintf can be considered as a
> > best practice. There is no overhead with this change and further if the
> > pattern is changed by someone in the future they might overlook the
> > buffersize
> 
> If we cut the string above we will give wrong information to the user space.
> I think scnprintf() change is a noise and does not improve the situation anyhow.

If, for any reason, any of the values are large the user will get
corrupt data.
But you don't want to leak random kernel memory to the user.

So while you may be able to prove that this particular snprintf()
can't overflow, in general checking it requires knowledge of the code.

With scnprintf() you know nothing odd will happen.

FWIW I suspect the 'standard' return value from snprintf() comes
from the return value of the original user-space implementations
which faked-up a FILE structure on stack and just silently discarded
the output bytes that wouldn't fit in the buffer (they'd usually
by flushed to a real file).
The original sprintf() just specified a very big length so the
flush would never be requested.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Re: [PATCH] iio: gyro: adis16136: use scnprintf instead of snprintf

['Andy Shevchenko']

On Mon, Mar 23, 2020 at 03:04:23PM +0000, David Laight wrote:
> From: Andy Shevchenko
> > Sent: 22 March 2020 10:27
> > On Sun, Mar 22, 2020 at 8:11 AM Rohit Sarkar <rohitsarkar5398@gmail.com> wrote:
> > >
> > > On Sun, Mar 22, 2020 at 02:25:42AM +0200, Andy Shevchenko wrote:
> > > > On Wed, Mar 18, 2020 at 08:25:22PM +0530, Rohit Sarkar wrote:
> > > > > scnprintf returns the actual number of bytes written into the buffer as
> > > > > opposed to snprintf which returns the number of bytes that would have
> > > > > been written if the buffer was big enough. Using the output of snprintf
> > > > > may lead to difficult to detect bugs.
> > > >
> > > > Nice. Have you investigate the code?
> > > >
> > > > > @@ -96,7 +96,7 @@ static ssize_t adis16136_show_serial(struct file *file,
> > > > >     if (ret)
> > > > >             return ret;
> > > > >
> > > > > -   len = snprintf(buf, sizeof(buf), "%.4x%.4x%.4x-%.4x\n", lot1, lot2,
> > > > > +   len = scnprintf(buf, sizeof(buf), "%.4x%.4x%.4x-%.4x\n", lot1, lot2,
> > > > >             lot3, serial);
> > > > >
> > > > >     return simple_read_from_buffer(userbuf, count, ppos, buf, len);
> > > >
> > > > The buffer size is 20, the pattern size I count to 19. Do you think snprintf()
> > > > can fail?
> > > That might be the case, but IMO using scnprintf can be considered as a
> > > best practice. There is no overhead with this change and further if the
> > > pattern is changed by someone in the future they might overlook the
> > > buffersize
> > 
> > If we cut the string above we will give wrong information to the user space.
> > I think scnprintf() change is a noise and does not improve the situation anyhow.
> 
> If, for any reason, any of the values are large the user will get
> corrupt data.

> But you don't want to leak random kernel memory to the user.

How? Kernel already got crashed at this point.

> 
> So while you may be able to prove that this particular snprintf()
> can't overflow, in general checking it requires knowledge of the code.

Here it's still a noise.

> With scnprintf() you know nothing odd will happen.

...and quite likely hide a lot of issues.

Really any "micro" / "small" correction / optimization to be very carefully
thought through.

> FWIW I suspect the 'standard' return value from snprintf() comes
> from the return value of the original user-space implementations
> which faked-up a FILE structure on stack and just silently discarded
> the output bytes that wouldn't fit in the buffer (they'd usually
> by flushed to a real file).
> The original sprintf() just specified a very big length so the
> flush would never be requested.

-- 
With Best Regards,
Andy Shevchenko