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

From: David Laight <David.Laight@ACULAB.COM>
To: 'Andy Shevchenko' <andy.shevchenko@gmail.com>,
	Rohit Sarkar <rohitsarkar5398@gmail.com>
Cc: Andy Shevchenko <andriy.shevchenko@intel.com>,
	linux-iio <linux-iio@vger.kernel.org>,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
	Jonathan Cameron <jic23@kernel.org>,
	"dragos.bogdan@analog.com" <dragos.bogdan@analog.com>,
	Lars-Peter Clausen <lars@metafoo.de>,
	Michael Hennerich <Michael.Hennerich@analog.com>,
	"Stefan Popa" <stefan.popa@analog.com>,
	Hartmut Knaack <knaack.h@gmx.de>,
	"Peter Meerwald" <pmeerw@pmeerw.net>
Subject: RE: [PATCH] iio: gyro: adis16136: use scnprintf instead of snprintf
Date: Mon, 23 Mar 2020 15:04:23 +0000	[thread overview]
Message-ID: <81821e142e3c48febb7ab357dd200992@AcuMS.aculab.com> (raw)
In-Reply-To: <CAHp75VddtJs1ZCk1XAZ2WJLhWQDcVwiiN5gDpK9oYPEOS=c_ZQ@mail.gmail.com>

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)