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