| Message ID | 5e723666.1c69fb81.3545b.79c3@mx.google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | iio: gyro: adis16136: use scnprintf instead of snprintf | expand |
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?
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 > >
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.
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)
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.
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);
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(-)