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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <aecd2e25900f2ef38f937a295e995269c433453b.camel@gmail.com>
Date: Tue, 28 Oct 2025 15:12:29 +0000
From: Nuno Sá <noname.nuno@...il.com>
To: Andy Shevchenko <andriy.shevchenko@...el.com>
Cc: Miaoqian Lin <linmq006@...il.com>, Markus Burri <markus.burri@...com>, 
 Lars-Peter Clausen
	 <lars@...afoo.de>, Michael Hennerich <Michael.Hennerich@...log.com>, 
 Jonathan Cameron
	 <jic23@...nel.org>, David Lechner <dlechner@...libre.com>, Nuno
 Sá
	 <nuno.sa@...log.com>, Andy Shevchenko <andy@...nel.org>, Angelo Dureghello
	 <adureghello@...libre.com>, linux-iio@...r.kernel.org, 
	linux-kernel@...r.kernel.org, stable@...r.kernel.org
Subject: Re: [PATCH] iio: dac: ad3552r-hs: fix out-of-bound write in
 ad3552r_hs_write_data_source

On Tue, 2025-10-28 at 16:45 +0200, Andy Shevchenko wrote:
> On Tue, Oct 28, 2025 at 12:31:04PM +0000, Nuno Sá wrote:
> > 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:
> 
> ...
> 
> > > > > > +	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).
> 
> It seems I stand corrected. I was staring too much at copy_from_user() without
> retrieving the validation logic behind simple_write_to_buffer().

:)

...

> > 
> > 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.
> 
> Yes, but it should either be explained as just making code robust vs. real
> bugfix.

Agreed. If we find it's the former, the commit message should be updated.

> For the latter I want to see the real traceback and a reproducer. I also
> wonder why
> we never had reports from syzkaller on this. It has non-zero chance to stumble
> over
> the issue here (if there is an issue to begin with).

If I have the time, I might do it. If my suspicious are correct, it should be
fairly easy to reproduce.

- Nuno Sá