Re: [PATCH] iio: dac: ad3552r-hs: fix out-of-bound write in ad3552r_hs_write_data_source

[林妙倩]

Hi, Andy

Thanks for you review.

Andy Shevchenko <andriy.shevchenko@xxxxxxxxx> 于2025年10月28日周二 17:07写道：
>
> On Tue, Oct 28, 2025 at 10:19:27AM +0200, Andy Shevchenko wrote:
> > On Tue, Oct 28, 2025 at 10:18:05AM +0200, Andy Shevchenko wrote:
> > > On Mon, Oct 27, 2025 at 11:07:13PM +0800, Miaoqian Lin wrote:
>
> +Cc: Markus Burri for the da9374819eb3
>
> ...
>
> > > > + if (count >= sizeof(buf))
> > > > +         return -ENOSPC;
> > >
> > > But this makes the validation too strict now.
> > >
> > > >   ret = simple_write_to_buffer(buf, sizeof(buf) - 1, ppos, userbuf,
> > > >                                count);
> > >
> > > You definitely failed to read the code that implements the above.
> > >

I previously read the simple_write_to_buffer(), but add the check and
think it can help to catch error eariler. My mistake.

> > > >   if (ret < 0)
> > > >           return ret;
> >
> > > > - buf[count] = '\0';
> > > > + buf[ret] = '\0';
> >
> > Maybe this line is what we might need, but I haven't checked deeper if it's a
> > problem.
>
> So, copy_to_user() and copy_from_user() are always inlined macros.
> The simple_write_to_buffer() is not. The question here is how
> the __builit_object_size() will behave on the address given as a parameter to
> copy_from_user() in simple_write_to_buffer().
>
> If it may detect reliably that the buffer is the size it has. I believe it's
> easy for the byte arrays on stack.
>
> That said, without proof that compiler is unable to determine the destination
> buffer size, this patch and the one by Markus are simple noise which actually
> changes an error code on the overflow condition.
>
> The only line that assigns NUL character might be useful in some cases
> (definitely when buffer comes through indirect calls from a heap, etc).
>

I believe it is still necessray to use buf[ret] = '\0'; intead of
buf[count] = '\0';
If you argee with this, I send a v2 with just this fix. Thanks.

> > > NAK.
> > >
> > > This patch is an unneeded churn.
>
> --
> With Best Regards,
> Andy Shevchenko
>
>