Re: EXTERNAL - [PATCH v1] iio: backend: fix out-of-bound write

[Jonathan Cameron <jic23@kernel.org>]

On Sun, 04 May 2025 09:27:02 +0100
Nuno Sá <noname.nuno@gmail.com> wrote:

> On Fri, 2025-05-02 at 23:11 +0200, Markus Burri wrote:
> > On Fri, May 02, 2025 at 04:12:04PM +0100, Nuno Sá wrote:  
> > > On Thu, 2025-05-01 at 08:32 +0200, Markus Burri wrote:  
> > > > The buffer is set to 80 character. If a caller write more characters,
> > > > count is truncated to the max available space in "simple_write_to_buffer".
> > > > But afterwards a string terminator is written to the buffer at offset count
> > > > without boundary check. The zero termination is written OUT-OF-BOUND.
> > > > 
> > > > Add a check that the given buffer is smaller then the buffer to prevent.
> > > > 
> > > > Fixes: 035b4989211d ("iio: backend: make sure to NULL terminate stack buffer")
> > > > Signed-off-by: Markus Burri <markus.burri@mt.com>
> > > > ---
> > > >  drivers/iio/industrialio-backend.c | 3 +++
> > > >  1 file changed, 3 insertions(+)
> > > > 
> > > > diff --git a/drivers/iio/industrialio-backend.c b/drivers/iio/industrialio-
> > > > backend.c
> > > > index a43c8d1bb3d0..3878bd698c98 100644
> > > > --- a/drivers/iio/industrialio-backend.c
> > > > +++ b/drivers/iio/industrialio-backend.c
> > > > @@ -155,6 +155,9 @@ static ssize_t iio_backend_debugfs_write_reg(struct file
> > > > *file,
> > > >  	ssize_t rc;
> > > >  	int ret;
> > > >  
> > > > +	if (count >= sizeof(buf))
> > > > +		return -ENOSPC;
> > > > +  
> > > 
> > > Oh, this can indeed easily lead to an oob access. However, I would likely not
> > > mind in early returning an error. This is to write registers so 80 should be
> > > more than enough. Meaning that to trigger this, it has to be intentional. That
> > > said, of course we should not let that happen but I would still truncate things
> > > and let it fail afterwards (keep the code slightly simpler with one less check).
> > >   
> > Thanks for your response.
> > I would prefer the upfront error check. The code is cleaner and simpler to read.  
> 
> Simpler yes, cleaner arguable :). Anyways, no strong feelings so leave it as-is if
> it's your preference. However, I think the proper way is:
> 
> if (count >= sizeof(buf) - 1)
> 	return -ENOSPC;
> 
> And since you're doing this, I think my suggestion still makes sense. I mean:
> 
> buf[rc] = '\0';
> 
> since rc is indeed the number of bytes written and we do want to terminate the buffer
> at the proper place. So the above is the correct form even if there's no real issue
> with the current form after this patch. Being this a fix, not sure if Jonathan is ok
> with the above in the current patch or as follow up.

Both together would be fine for this as it's related stuff.

I like the combination of the two though agree only the initial check is strictly
necessary (though the - 1 is needed to avoid scrubbing the 80th char and rather
unexpected results.)

J
> 
> - Nuno Sá
> 
> > As you wrote the buffer should only contain a register and a value and
> > therefore never extend the 80 character.
> > If there are more, it must be intentional or by mistake. In both cases I expect
> > to get an error back, instead of try to handle partial/wrong data.
> >   
> > > So I would instead do:
> > > 
> > > buf[rc] = '\0';
> > > 
> > > Thanks for catching this!
> > > - Nuno Sá
> > > 
> > >   
> > > >  	rc = simple_write_to_buffer(buf, sizeof(buf) - 1, ppos, userbuf,
> > > > count);
> > > >  	if (rc < 0)
> > > >  		return rc;
> > > > 
> > > > base-commit: b4432656b36e5cc1d50a1f2dc15357543add530e  
>