On Sun, 15 Mar 2020 10:58:34 +0100,
Jonathan Cameron wrote:
>
> On Wed, 11 Mar 2020 08:43:25 +0100
> Takashi Iwai <tiwai@xxxxxxx> 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@xxxxxxx>
>
> 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.
Well, the issue handled here isn't whether the string is truncated.
Rather whether the pointer passed to the function go beyond the
boundary.
In short, the call pattern like
pos += snprintf(buf, limit - pos, ...);
is wrong per design of snprintf(), unless you check the return value
at each time. snprintf() doesn't return the actually truncated length
but the length that would be. So, concatenating with snprintf() can
go beyond the limit.
Meanwhile, simply replacing it with scnprintf() avoids the wrong
pointer increment effectively, as it returns the actual output size,
so it'd return zero if it's all truncated.
Practically seen, the current code should "work", but it's a wrong
usage of snprintf(), as found in many other places. I've already
submitted the patches to each subsystem to cover at least the
concatenating cases like the above, and this is one of them.
thanks,
Takashi
>
> Brian, can you take a look at this when you get a moment?
>
> 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;
> > }
> >
>
