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: <c3a792558b7d35f7342a53c94810861cb0eb0b8e.camel@gmail.com>
Date: Fri, 02 May 2025 16:12:04 +0100
From: Nuno Sá <noname.nuno@...il.com>
To: Markus Burri <markus.burri@...com>, linux-kernel@...r.kernel.org
Cc: Nuno Sa <nuno.sa@...log.com>, Olivier Moysan
 <olivier.moysan@...s.st.com>,  Jonathan Cameron	 <jic23@...nel.org>,
 Lars-Peter Clausen <lars@...afoo.de>, 	linux-iio@...r.kernel.org, Markus
 Burri <markus.burri@....ch>
Subject: Re: [PATCH v1] iio: backend: fix out-of-bound write

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

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ