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

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

Posted by Markus Burri 9 months, 1 week ago

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;
+
 	rc = simple_write_to_buffer(buf, sizeof(buf) - 1, ppos, userbuf, count);
 	if (rc < 0)
 		return rc;

base-commit: b4432656b36e5cc1d50a1f2dc15357543add530e
-- 
2.39.5

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

Posted by Nuno Sá 9 months, 1 week ago

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

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

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

Posted by Markus Burri 9 months, 1 week ago

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

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

Posted by Nuno Sá 9 months, 1 week ago

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.

- 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

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

Posted by Jonathan Cameron 9 months, 1 week ago

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  
>