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

[Andy Shevchenko <andriy.shevchenko@intel.com>]

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.
> > 
> > >  	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).

> > NAK.
> > 
> > This patch is an unneeded churn.

-- 
With Best Regards,
Andy Shevchenko