netconsole: avoid out-of-bounds access on empty string in trim_newline()

[PATCH net] netconsole: avoid out-of-bounds access on empty string in trim_newline()

Posted by Breno Leitao 1 month, 3 weeks ago

trim_newline() unconditionally dereferences s[len - 1] after computing
len = strnlen(s, maxlen). When the string is empty, len is 0 and the
expression underflows to s[(size_t)-1], reading (and potentially
writing) one byte before the buffer.

The two callers feed trim_newline() with the result of strscpy() from
configfs store callbacks (dev_name_store, userdatum_value_store).
configfs guarantees count >= 1 reaches the callback, but the byte
itself can be NUL: a userspace write(fd, "\0", 1) leaves the
destination empty after strscpy() and triggers the underflow. The OOB
write only fires if the adjacent byte happens to be '\n', so this is
not a security issue, but the access is undefined behaviour either way.

This pattern is commonly flagged by LLM-based code reviewers. While it
is not a security fix, the underlying access is undefined behaviour and
the change is small and self-contained, so it is a reasonable candidate
for the stable trees.

Guard the dereference on a non-zero length.

Fixes: ae001dc67907 ("net: netconsole: move newline trimming to function")
Cc: stable@vger.kernel.org
Signed-off-by: Breno Leitao <leitao@debian.org>
---
 drivers/net/netconsole.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/net/netconsole.c b/drivers/net/netconsole.c
index 3c9acd6e49e86..205384dab89a6 100644
--- a/drivers/net/netconsole.c
+++ b/drivers/net/netconsole.c
@@ -497,6 +497,8 @@ static void trim_newline(char *s, size_t maxlen)
 	size_t len;
 
 	len = strnlen(s, maxlen);
+	if (!len)
+		return;
 	if (s[len - 1] == '\n')
 		s[len - 1] = '\0';
 }

---
base-commit: c7275b05bc428c7373d97aa2da02d3a7fa6b9f66
change-id: 20260420-netcons_trim_newline-36f6ec3b9820

Best regards,
--  
Breno Leitao <leitao@debian.org>

Re: [PATCH net] netconsole: avoid out-of-bounds access on empty string in trim_newline()

Posted by Simon Horman 1 month, 3 weeks ago

On Mon, Apr 20, 2026 at 03:18:36AM -0700, Breno Leitao wrote:
> trim_newline() unconditionally dereferences s[len - 1] after computing
> len = strnlen(s, maxlen). When the string is empty, len is 0 and the
> expression underflows to s[(size_t)-1], reading (and potentially
> writing) one byte before the buffer.
> 
> The two callers feed trim_newline() with the result of strscpy() from
> configfs store callbacks (dev_name_store, userdatum_value_store).
> configfs guarantees count >= 1 reaches the callback, but the byte
> itself can be NUL: a userspace write(fd, "\0", 1) leaves the
> destination empty after strscpy() and triggers the underflow. The OOB
> write only fires if the adjacent byte happens to be '\n', so this is
> not a security issue, but the access is undefined behaviour either way.
> 
> This pattern is commonly flagged by LLM-based code reviewers. While it
> is not a security fix, the underlying access is undefined behaviour and
> the change is small and self-contained, so it is a reasonable candidate
> for the stable trees.
> 
> Guard the dereference on a non-zero length.
> 
> Fixes: ae001dc67907 ("net: netconsole: move newline trimming to function")
> Cc: stable@vger.kernel.org
> Signed-off-by: Breno Leitao <leitao@debian.org>

Reviewed-by: Simon Horman <horms@kernel.org>

Sashiko has provided some feedback on this patch.
I do not believe that should hold up progress of this patch.
But I'd appreciate it if you could look over that feedback
and see if any follow-up is warranted.

Thanks!

Re: [PATCH net] netconsole: avoid out-of-bounds access on empty string in trim_newline()

Posted by Breno Leitao 1 month, 3 weeks ago

On Tue, Apr 21, 2026 at 05:22:19PM +0100, Simon Horman wrote:
> On Mon, Apr 20, 2026 at 03:18:36AM -0700, Breno Leitao wrote:
> > trim_newline() unconditionally dereferences s[len - 1] after computing
> > len = strnlen(s, maxlen). When the string is empty, len is 0 and the
> > expression underflows to s[(size_t)-1], reading (and potentially
> > writing) one byte before the buffer.
> > 
> > The two callers feed trim_newline() with the result of strscpy() from
> > configfs store callbacks (dev_name_store, userdatum_value_store).
> > configfs guarantees count >= 1 reaches the callback, but the byte
> > itself can be NUL: a userspace write(fd, "\0", 1) leaves the
> > destination empty after strscpy() and triggers the underflow. The OOB
> > write only fires if the adjacent byte happens to be '\n', so this is
> > not a security issue, but the access is undefined behaviour either way.
> > 
> > This pattern is commonly flagged by LLM-based code reviewers. While it
> > is not a security fix, the underlying access is undefined behaviour and
> > the change is small and self-contained, so it is a reasonable candidate
> > for the stable trees.
> > 
> > Guard the dereference on a non-zero length.
> > 
> > Fixes: ae001dc67907 ("net: netconsole: move newline trimming to function")
> > Cc: stable@vger.kernel.org
> > Signed-off-by: Breno Leitao <leitao@debian.org>
> 
> Reviewed-by: Simon Horman <horms@kernel.org>
> 
> Sashiko has provided some feedback on this patch.
> I do not believe that should hold up progress of this patch.
> But I'd appreciate it if you could look over that feedback
> and see if any follow-up is warranted.

Thanks for the review, I've had a quick look, and it is complaining
about problems are not regressions, but some other issues in the code,
which I will need to check more carefully tomorrow.

https://sashiko.dev/#/patchset/20260420-netcons_trim_newline-v1-1-dc35889aeedf%40debian.org

Thanks,
--breno

Re: [PATCH net] netconsole: avoid out-of-bounds access on empty string in trim_newline()

Posted by Gustavo Luiz Duarte 1 month, 3 weeks ago

On Mon, Apr 20, 2026 at 11:34 AM Breno Leitao <leitao@debian.org> wrote:
>
> trim_newline() unconditionally dereferences s[len - 1] after computing
> len = strnlen(s, maxlen). When the string is empty, len is 0 and the
> expression underflows to s[(size_t)-1], reading (and potentially
> writing) one byte before the buffer.
>
> The two callers feed trim_newline() with the result of strscpy() from
> configfs store callbacks (dev_name_store, userdatum_value_store).
> configfs guarantees count >= 1 reaches the callback, but the byte
> itself can be NUL: a userspace write(fd, "\0", 1) leaves the
> destination empty after strscpy() and triggers the underflow. The OOB
> write only fires if the adjacent byte happens to be '\n', so this is
> not a security issue, but the access is undefined behaviour either way.
>
> This pattern is commonly flagged by LLM-based code reviewers. While it
> is not a security fix, the underlying access is undefined behaviour and
> the change is small and self-contained, so it is a reasonable candidate
> for the stable trees.
>
> Guard the dereference on a non-zero length.
>
> Fixes: ae001dc67907 ("net: netconsole: move newline trimming to function")
> Cc: stable@vger.kernel.org
> Signed-off-by: Breno Leitao <leitao@debian.org>

Reviewed-by: Gustavo Luiz Duarte <gustavold@gmail.com>

> ---
>  drivers/net/netconsole.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/drivers/net/netconsole.c b/drivers/net/netconsole.c
> index 3c9acd6e49e86..205384dab89a6 100644
> --- a/drivers/net/netconsole.c
> +++ b/drivers/net/netconsole.c
> @@ -497,6 +497,8 @@ static void trim_newline(char *s, size_t maxlen)
>         size_t len;
>
>         len = strnlen(s, maxlen);
> +       if (!len)
> +               return;
>         if (s[len - 1] == '\n')
>                 s[len - 1] = '\0';
>  }
>
> ---
> base-commit: c7275b05bc428c7373d97aa2da02d3a7fa6b9f66
> change-id: 20260420-netcons_trim_newline-36f6ec3b9820
>
> Best regards,
> --
> Breno Leitao <leitao@debian.org>
>
>