Linux-IIO Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH] iio: gyro: adis16136: use scnprintf instead of snprintf
@ 2020-03-18 14:55 Rohit Sarkar
  2020-03-22  0:25 ` Andy Shevchenko
  0 siblings, 1 reply; 6+ messages in thread
From: Rohit Sarkar @ 2020-03-18 14:55 UTC (permalink / raw)
  To: linux-iio, linux-kernel
  Cc: jic23, rohitsarkar5398, dragos.bogdan, Lars-Peter Clausen,
	Michael Hennerich, Stefan Popa, knaack.h, pmeerw

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


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] iio: gyro: adis16136: use scnprintf instead of snprintf
  2020-03-18 14:55 [PATCH] iio: gyro: adis16136: use scnprintf instead of snprintf Rohit Sarkar
@ 2020-03-22  0:25 ` Andy Shevchenko
  2020-03-22  6:11   ` Rohit Sarkar
  0 siblings, 1 reply; 6+ messages in thread
From: Andy Shevchenko @ 2020-03-22  0:25 UTC (permalink / raw)
  To: Rohit Sarkar
  Cc: linux-iio, linux-kernel, jic23, dragos.bogdan,
	Lars-Peter Clausen, Michael Hennerich, Stefan Popa, knaack.h,
	pmeerw

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



^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] iio: gyro: adis16136: use scnprintf instead of snprintf
  2020-03-22  0:25 ` Andy Shevchenko
@ 2020-03-22  6:11   ` Rohit Sarkar
  2020-03-22 10:27     ` Andy Shevchenko
  0 siblings, 1 reply; 6+ messages in thread
From: Rohit Sarkar @ 2020-03-22  6:11 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: linux-iio, linux-kernel, jic23, dragos.bogdan,
	Lars-Peter Clausen, Michael Hennerich, Stefan Popa, knaack.h,
	pmeerw

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
> 
> 

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] iio: gyro: adis16136: use scnprintf instead of snprintf
  2020-03-22  6:11   ` Rohit Sarkar
@ 2020-03-22 10:27     ` Andy Shevchenko
  2020-03-23 15:04       ` David Laight
  0 siblings, 1 reply; 6+ messages in thread
From: Andy Shevchenko @ 2020-03-22 10:27 UTC (permalink / raw)
  To: Rohit Sarkar
  Cc: Andy Shevchenko, linux-iio, Linux Kernel Mailing List,
	Jonathan Cameron, dragos.bogdan, Lars-Peter Clausen,
	Michael Hennerich, Stefan Popa, Hartmut Knaack, Peter Meerwald

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

^ permalink raw reply	[flat|nested] 6+ messages in thread

* RE: [PATCH] iio: gyro: adis16136: use scnprintf instead of snprintf
  2020-03-22 10:27     ` Andy Shevchenko
@ 2020-03-23 15:04       ` David Laight
  2020-03-23 16:05         ` 'Andy Shevchenko'
  0 siblings, 1 reply; 6+ messages in thread
From: David Laight @ 2020-03-23 15:04 UTC (permalink / raw)
  To: 'Andy Shevchenko', Rohit Sarkar
  Cc: Andy Shevchenko, linux-iio, Linux Kernel Mailing List,
	Jonathan Cameron, dragos.bogdan, Lars-Peter Clausen,
	Michael Hennerich, Stefan Popa, Hartmut Knaack, Peter Meerwald

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)

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] iio: gyro: adis16136: use scnprintf instead of snprintf
  2020-03-23 15:04       ` David Laight
@ 2020-03-23 16:05         ` 'Andy Shevchenko'
  0 siblings, 0 replies; 6+ messages in thread
From: 'Andy Shevchenko' @ 2020-03-23 16:05 UTC (permalink / raw)
  To: David Laight
  Cc: Rohit Sarkar, linux-iio, Linux Kernel Mailing List,
	Jonathan Cameron, dragos.bogdan, Lars-Peter Clausen,
	Michael Hennerich, Stefan Popa, Hartmut Knaack, Peter Meerwald

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



^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, back to index

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-03-18 14:55 [PATCH] iio: gyro: adis16136: use scnprintf instead of snprintf Rohit Sarkar
2020-03-22  0:25 ` Andy Shevchenko
2020-03-22  6:11   ` Rohit Sarkar
2020-03-22 10:27     ` Andy Shevchenko
2020-03-23 15:04       ` David Laight
2020-03-23 16:05         ` 'Andy Shevchenko'

Linux-IIO Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-iio/0 linux-iio/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-iio linux-iio/ https://lore.kernel.org/linux-iio \
		linux-iio@vger.kernel.org
	public-inbox-index linux-iio

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-iio


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git