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

[Nuno Sá]

On Tue, 2025-10-28 at 11:07 +0200, Andy Shevchenko wrote:
> 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.
> 

I think the above does not make sense (unless I'm missing your point which might
very well be). So, __builit_object_size() is for things known at compile time.
Moreover, simple_write_to_buffer() already has all of the gymnastics to make
sure copy_from_user() has the proper parameters. The thing is that it does it in
a "silent" way which means that if your buffer is not big enough you'll get a
concatenated string. Sure, you'll likely get an error down the road (due to an
invalid value) but I do see some value in returning back the root cause of the
issue.

So, the preliminary check while not being a big deal, it's also not completely
useless IMO. I do not have any strong feeling though. However, I think the below
is very much needed...

> 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 think you can easily pass a string >= than 64 bytes (from userspace). AFAIR,
you don't really set a size into debugfs files. For sure you can mess things
with zero sized binary attributes so I have some confidence you have the same
with debugfs.

And even if all the above is not reproducible I'm still of the opinion that

buf[ret] = '\0';

is semantically the correct code.

- Nuno Sá