Re: [PATCH 2/2] iio: tsl2772: Use scnprintf() for avoiding potential buffer overflow

[Jonathan Cameron <Jonathan.Cameron@Huawei.com>]

On Mon, 16 Mar 2020 09:04:46 +0100
Takashi Iwai <tiwai@suse.de> wrote:

> On Sun, 15 Mar 2020 14:10:08 +0100,
> Jonathan Cameron wrote:
> > 
> > On Sun, 15 Mar 2020 06:33:58 -0400
> > Brian Masney <masneyb@onstation.org> wrote:
> >   
> > > On Sun, Mar 15, 2020 at 09:58:34AM +0000, Jonathan Cameron wrote:  
> > > > On Wed, 11 Mar 2020 08:43:25 +0100
> > > > Takashi Iwai <tiwai@suse.de> wrote:
> > > >     
> > > > > Since snprintf() returns the would-be-output size instead of the
> > > > > actual output size, the succeeding calls may go beyond the given
> > > > > buffer limit.  Fix it by replacing with scnprintf().
> > > > > 
> > > > > Signed-off-by: Takashi Iwai <tiwai@suse.de>    
> > > > 
> > > > This one is printing a short well defined list of values.  No way they go
> > > > anywhere near the smallest possible PAGE_SIZE buffer that it's printing
> > > > into.
> > > > 
> > > > Which is handy given the remaining space isn't adjusted as we add items
> > > > to the string.  Hence even with scnprintf it would overflow.
> > > > 
> > > > Brian, can you take a look at this when you get a moment?    
> > > 
> > > I also agree that this won't overflow in practice, however we should fix
> > > this up. Maybe the scnprintf() calls should be this:
> > > 
> > >     offset += scnprintf(buf + offset, PAGE_SIZE - offset, ...);  
> > 
> > Agreed.  
> 
> Oh indeed, that must be.  This was the totally overlooked point.
> So the code has to be corrected altogether.
> 
> Shall I respin the patch with that change?

That would be great!

It's always amazing what turns up once you start looking closely at
a few lines of code :)

Jonathan

> 
> 
> thanks,
> 
> Takashi
> 
> > 
> > Jonathan
> >   
> > > 
> > > Brian
> > > 
> > >   
> > > > 
> > > > Thanks,
> > > > 
> > > > Jonathan
> > > >     
> > > > > ---
> > > > >  drivers/iio/light/tsl2772.c | 4 ++--
> > > > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/iio/light/tsl2772.c b/drivers/iio/light/tsl2772.c
> > > > > index be37fcbd4654..44a0b56a558c 100644
> > > > > --- a/drivers/iio/light/tsl2772.c
> > > > > +++ b/drivers/iio/light/tsl2772.c
> > > > > @@ -986,7 +986,7 @@ static ssize_t in_illuminance0_lux_table_show(struct device *dev,
> > > > >  	int offset = 0;
> > > > >  
> > > > >  	while (i < TSL2772_MAX_LUX_TABLE_SIZE) {
> > > > > -		offset += snprintf(buf + offset, PAGE_SIZE, "%u,%u,",
> > > > > +		offset += scnprintf(buf + offset, PAGE_SIZE, "%u,%u,",
> > > > >  			chip->tsl2772_device_lux[i].ch0,
> > > > >  			chip->tsl2772_device_lux[i].ch1);
> > > > >  		if (chip->tsl2772_device_lux[i].ch0 == 0) {
> > > > > @@ -1000,7 +1000,7 @@ static ssize_t in_illuminance0_lux_table_show(struct device *dev,
> > > > >  		i++;
> > > > >  	}
> > > > >  
> > > > > -	offset += snprintf(buf + offset, PAGE_SIZE, "\n");
> > > > > +	offset += scnprintf(buf + offset, PAGE_SIZE, "\n");
> > > > >  	return offset;
> > > > >  }
> > > > >      
> >