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 for Android: free password hash cracker in your pocket

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

Message-ID: <aQCT0q4hKtlVbJDU@smile.fi.intel.com>
Date: Tue, 28 Oct 2025 11:58:42 +0200
From: Andy Shevchenko <andriy.shevchenko@...el.com>
To: 林妙倩 <linmq006@...il.com>
Cc: 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, Oct 28, 2025 at 05:46:53PM +0800, 林妙倩 wrote:
> Andy Shevchenko <andriy.shevchenko@...el.com> 于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:

...

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

As explained above, please try to model the situation and see if current code
is buggy, i.e. provide a step-by-step test case and show a traceback that
points to a out-of-boundary access in this function. (Note, you don't need to
have a HW for that, you might need to create a dummy IIO or other module with
the similar interface and run it, in such a case, share also link to the source
code of that module.) When you prove the problem exists, I will happily ACK
all similar patches, including yours.

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

-- 
With Best Regards,
Andy Shevchenko