Thread (11 messages) 11 messages, 2 authors, 24d ago

Re: [PATCH net 2/3] netconsole: avoid clobbering userdatum value on truncated write

From: Breno Leitao <leitao@debian.org>
Date: 2026-04-27 10:51:43
Also in: netdev

On Sun, Apr 26, 2026 at 09:35:16AM +0100, Simon Horman wrote:
From: 'Simon Horman' <horms@kernel.org>

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
Full review at: https://sashiko.dev
---
netconsole: avoid clobbering userdatum value on truncated write

This patch tightens the boundary check on userdatum values to prevent
strscpy() from returning -E2BIG and leaving a truncated string in udm->value.
quoted
diff --git a/drivers/net/netconsole.c b/drivers/net/netconsole.c
index 5713cb3783ef2..4bef003d9df64 100644
--- a/drivers/net/netconsole.c
+++ b/drivers/net/netconsole.c
@@ -1074,7 +1074,7 @@ static ssize_t userdatum_value_store(struct config_item *item, const char *buf,
 	struct userdata *ud;
 	ssize_t ret;
 
-	if (count > MAX_EXTRADATA_VALUE_LEN)
+	if (count >= MAX_EXTRADATA_VALUE_LEN)
 		return -EMSGSIZE;
 
 	mutex_lock(&netconsole_subsys.su_mutex);
This patch addresses the state corruption caused by strscpy() failing, but does
this fully resolve the issue?

The commit message notes that returning an error after modifying udm->value
leaves the internal state out of sync until the next write. Looking slightly
further down in userdatum_value_store():

userdatum_value_store() {
        ...
        ret = strscpy(udm->value, buf, sizeof(udm->value));
        if (ret < 0)
                goto out_unlock;
        trim_newline(udm->value, sizeof(udm->value));

        ud = to_userdata(item->ci_parent);
        nt = userdata_to_target(ud);
        ret = update_userdata(nt);
        if (ret < 0)
                goto out_unlock;
        ...
}

If update_userdata() fails, the function jumps to out_unlock and returns an
error to userspace. Since udm->value has already been updated with the new
string, the next successful write to any entry on this target will unknowingly
pack this new value into the payload, recreating the same deferred-activation
issue.
Right, this seems to be an issue where udm->value continues to be set,
even when update_userdata() fails.

The correct approach here seems to use a bouncy bufferr.

Let me create add this exta fix in the v2.
--breno
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help