* [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 related [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, other threads:[~2020-03-23 16:05 UTC | newest]
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'
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).