[PATCH net 0/3] netconsole: configfs store callback fixes

[PATCH net 0/3] netconsole: configfs store callback fixes

[Breno Leitao]

This series fixes a small cluster of related issues in netconsole's
configfs store callbacks.

They showed up in sashiko and brought to my attention by Simon:

https://lore.kernel.org/all/20260421162219.GF651125@horms.kernel.org/

None are crashes or security problems, but each is a real correctness
bug that surfaces at boundary conditions and was easy to clean up while
the code was already under the microscope.

All three changes narrow the accepted write size by exactly one byte
at the boundary that was previously buggy, so no well-behaved
userspace should notice the tightening.

Signed-off-by: Breno Leitao <leitao@debian.org>
---
Breno Leitao (3):
      netconsole: return count instead of strnlen(buf, count) from store callbacks
      netconsole: avoid clobbering userdatum value on truncated write
      netconsole: propagate device name truncation in dev_name_store()

 drivers/net/netconsole.c | 31 +++++++++++++++++--------------
 1 file changed, 17 insertions(+), 14 deletions(-)
---
base-commit: 70c8a7ec6715b5fb14e501731b5b9210a16684f7
change-id: 20260422-netconsole_ai_fixes-24599337a79d

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

[PATCH net 1/3] netconsole: return count instead of strnlen(buf, count) from store callbacks

[Breno Leitao]

Several configfs store callbacks in netconsole end with:

	ret = strnlen(buf, count);

This under-reports the number of bytes consumed when the input
contains an embedded NUL within count, telling the VFS that fewer
bytes were written than userspace actually handed in. A conformant
partial-write loop would then retry the trailing bytes against a
callback that has already accepted them.

Every other configfs driver in the tree returns count directly from
its store callbacks once parsing has succeeded, including
drivers/nvme/target/configfs.c, drivers/gpio/gpio-sim.c,
drivers/most/configfs.c, drivers/block/null_blk/main.c,
drivers/pci/endpoint/pci-ep-cfs.c, and the rest of the configfs
users. netconsole was the outlier (along with
drivers/infiniband/core/cma_configfs.c, which has the same latent
issue).

Align netconsole with the rest of the configfs ecosystem: return
count once the parser/validator has accepted the input. The numeric
and boolean parsers (kstrtobool, kstrtou16, mac_pton,
netpoll_parse_ip_addr) have already validated the meaningful prefix;
any trailing bytes are padding and should simply be reported as
consumed.

Fixes: 0bcc1816188e ("[NET] netconsole: Support dynamic reconfiguration using configfs")
Signed-off-by: Breno Leitao <leitao@debian.org>
---
 drivers/net/netconsole.c | 26 +++++++++++++-------------
 1 file changed, 13 insertions(+), 13 deletions(-)

diff --git a/drivers/net/netconsole.c b/drivers/net/netconsole.c
index 3c9acd6e49e86..5713cb3783ef2 100644
--- a/drivers/net/netconsole.c
+++ b/drivers/net/netconsole.c
@@ -750,7 +750,7 @@ static ssize_t enabled_store(struct config_item *item,
 		unregister_netcons_consoles();
 	}
 
-	ret = strnlen(buf, count);
+	ret = count;
 	/* Deferred cleanup */
 	netconsole_process_cleanups();
 out_unlock:
@@ -779,7 +779,7 @@ static ssize_t release_store(struct config_item *item, const char *buf,
 
 	nt->release = release;
 
-	ret = strnlen(buf, count);
+	ret = count;
 out_unlock:
 	dynamic_netconsole_mutex_unlock();
 	return ret;
@@ -805,7 +805,7 @@ static ssize_t extended_store(struct config_item *item, const char *buf,
 		goto out_unlock;
 
 	nt->extended = extended;
-	ret = strnlen(buf, count);
+	ret = count;
 out_unlock:
 	dynamic_netconsole_mutex_unlock();
 	return ret;
@@ -828,7 +828,7 @@ static ssize_t dev_name_store(struct config_item *item, const char *buf,
 	trim_newline(nt->np.dev_name, IFNAMSIZ);
 
 	dynamic_netconsole_mutex_unlock();
-	return strnlen(buf, count);
+	return count;
 }
 
 static ssize_t local_port_store(struct config_item *item, const char *buf,
@@ -847,7 +847,7 @@ static ssize_t local_port_store(struct config_item *item, const char *buf,
 	ret = kstrtou16(buf, 10, &nt->np.local_port);
 	if (ret < 0)
 		goto out_unlock;
-	ret = strnlen(buf, count);
+	ret = count;
 out_unlock:
 	dynamic_netconsole_mutex_unlock();
 	return ret;
@@ -869,7 +869,7 @@ static ssize_t remote_port_store(struct config_item *item,
 	ret = kstrtou16(buf, 10, &nt->np.remote_port);
 	if (ret < 0)
 		goto out_unlock;
-	ret = strnlen(buf, count);
+	ret = count;
 out_unlock:
 	dynamic_netconsole_mutex_unlock();
 	return ret;
@@ -894,7 +894,7 @@ static ssize_t local_ip_store(struct config_item *item, const char *buf,
 		goto out_unlock;
 	nt->np.ipv6 = !!ipv6;
 
-	ret = strnlen(buf, count);
+	ret = count;
 out_unlock:
 	dynamic_netconsole_mutex_unlock();
 	return ret;
@@ -919,7 +919,7 @@ static ssize_t remote_ip_store(struct config_item *item, const char *buf,
 		goto out_unlock;
 	nt->np.ipv6 = !!ipv6;
 
-	ret = strnlen(buf, count);
+	ret = count;
 out_unlock:
 	dynamic_netconsole_mutex_unlock();
 	return ret;
@@ -955,7 +955,7 @@ static ssize_t remote_mac_store(struct config_item *item, const char *buf,
 		goto out_unlock;
 	memcpy(nt->np.remote_mac, remote_mac, ETH_ALEN);
 
-	ret = strnlen(buf, count);
+	ret = count;
 out_unlock:
 	dynamic_netconsole_mutex_unlock();
 	return ret;
@@ -1131,7 +1131,7 @@ static ssize_t sysdata_msgid_enabled_store(struct config_item *item,
 		disable_sysdata_feature(nt, SYSDATA_MSGID);
 
 unlock_ok:
-	ret = strnlen(buf, count);
+	ret = count;
 	dynamic_netconsole_mutex_unlock();
 	mutex_unlock(&netconsole_subsys.su_mutex);
 	return ret;
@@ -1160,7 +1160,7 @@ static ssize_t sysdata_release_enabled_store(struct config_item *item,
 		disable_sysdata_feature(nt, SYSDATA_RELEASE);
 
 unlock_ok:
-	ret = strnlen(buf, count);
+	ret = count;
 	dynamic_netconsole_mutex_unlock();
 	mutex_unlock(&netconsole_subsys.su_mutex);
 	return ret;
@@ -1189,7 +1189,7 @@ static ssize_t sysdata_taskname_enabled_store(struct config_item *item,
 		disable_sysdata_feature(nt, SYSDATA_TASKNAME);
 
 unlock_ok:
-	ret = strnlen(buf, count);
+	ret = count;
 	dynamic_netconsole_mutex_unlock();
 	mutex_unlock(&netconsole_subsys.su_mutex);
 	return ret;
@@ -1223,7 +1223,7 @@ static ssize_t sysdata_cpu_nr_enabled_store(struct config_item *item,
 		disable_sysdata_feature(nt, SYSDATA_CPU_NR);
 
 unlock_ok:
-	ret = strnlen(buf, count);
+	ret = count;
 	dynamic_netconsole_mutex_unlock();
 	mutex_unlock(&netconsole_subsys.su_mutex);
 	return ret;

-- 
2.52.0

Re: [PATCH net 1/3] netconsole: return count instead of strnlen(buf, count) from store callbacks

[Simon Horman]

On Thu, Apr 23, 2026 at 02:41:15AM -0700, Breno Leitao wrote:
> Several configfs store callbacks in netconsole end with:
> 
> 	ret = strnlen(buf, count);
> 
> This under-reports the number of bytes consumed when the input
> contains an embedded NUL within count, telling the VFS that fewer
> bytes were written than userspace actually handed in. A conformant
> partial-write loop would then retry the trailing bytes against a
> callback that has already accepted them.
> 
> Every other configfs driver in the tree returns count directly from
> its store callbacks once parsing has succeeded, including
> drivers/nvme/target/configfs.c, drivers/gpio/gpio-sim.c,
> drivers/most/configfs.c, drivers/block/null_blk/main.c,
> drivers/pci/endpoint/pci-ep-cfs.c, and the rest of the configfs
> users. netconsole was the outlier (along with
> drivers/infiniband/core/cma_configfs.c, which has the same latent
> issue).
> 
> Align netconsole with the rest of the configfs ecosystem: return
> count once the parser/validator has accepted the input. The numeric
> and boolean parsers (kstrtobool, kstrtou16, mac_pton,
> netpoll_parse_ip_addr) have already validated the meaningful prefix;
> any trailing bytes are padding and should simply be reported as
> consumed.
> 
> Fixes: 0bcc1816188e ("[NET] netconsole: Support dynamic reconfiguration using configfs")
> Signed-off-by: Breno Leitao <leitao@debian.org>

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

FTR: Sashiko has provided an AI generated review of this patch.
Like it's review of patch 3/3 - which I forwarded separately -
it flags that trim_newline() may perform an OOB access
if passed an empty string. But this is not correct because
trim_newline() correctly handles this case.

Re: [PATCH net 1/3] netconsole: return count instead of strnlen(buf, count) from store callbacks

[Breno Leitao]

Hello Simon,

On Sun, Apr 26, 2026 at 09:46:40AM +0100, Simon Horman wrote:
> On Thu, Apr 23, 2026 at 02:41:15AM -0700, Breno Leitao wrote:
> > Several configfs store callbacks in netconsole end with:
> >
> > 	ret = strnlen(buf, count);
> >
> > This under-reports the number of bytes consumed when the input
> > contains an embedded NUL within count, telling the VFS that fewer
> > bytes were written than userspace actually handed in. A conformant
> > partial-write loop would then retry the trailing bytes against a
> > callback that has already accepted them.
> >
> > Every other configfs driver in the tree returns count directly from
> > its store callbacks once parsing has succeeded, including
> > drivers/nvme/target/configfs.c, drivers/gpio/gpio-sim.c,
> > drivers/most/configfs.c, drivers/block/null_blk/main.c,
> > drivers/pci/endpoint/pci-ep-cfs.c, and the rest of the configfs
> > users. netconsole was the outlier (along with
> > drivers/infiniband/core/cma_configfs.c, which has the same latent
> > issue).
> >
> > Align netconsole with the rest of the configfs ecosystem: return
> > count once the parser/validator has accepted the input. The numeric
> > and boolean parsers (kstrtobool, kstrtou16, mac_pton,
> > netpoll_parse_ip_addr) have already validated the meaningful prefix;
> > any trailing bytes are padding and should simply be reported as
> > consumed.
> >
> > Fixes: 0bcc1816188e ("[NET] netconsole: Support dynamic reconfiguration using configfs")
> > Signed-off-by: Breno Leitao <leitao@debian.org>
>
> Reviewed-by: Simon Horman <horms@kernel.org>
>
> FTR: Sashiko has provided an AI generated review of this patch.
> Like it's review of patch 3/3 - which I forwarded separately -
> it flags that trim_newline() may perform an OOB access
> if passed an empty string. But this is not correct because
> trim_newline() correctly handles this case.

You're absolutely right. This has been addressed in the patch that recently
landed in Linus' tree.

It appears Sashiko ran its analysis on a tree predating that fix.

https://github.com/torvalds/linux/commit/7079c8c13f2d33992bc846240517d88f4ab07781

Thanks for the review,
--breno

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

[Breno Leitao]

userdatum_value_store() bounds count by MAX_EXTRADATA_VALUE_LEN (200)
and then copies straight into udm->value, which is itself 200 bytes:

	if (count > MAX_EXTRADATA_VALUE_LEN)
		return -EMSGSIZE;
	...
	ret = strscpy(udm->value, buf, sizeof(udm->value));
	if (ret < 0)
		goto out_unlock;

If userspace writes exactly MAX_EXTRADATA_VALUE_LEN bytes with no NUL
within them, strscpy() copies 199 bytes plus a NUL into udm->value and
returns -E2BIG. The function jumps to out_unlock and reports the error
to userspace, but udm->value has already been overwritten with the
truncated string and update_userdata() is skipped, so the corruption
is not yet visible on the wire.

The next successful write to any userdatum entry under the same target
calls update_userdata(), which packs udm->value into the active
netconsole payload. From that point on, every netconsole message
carries the silently truncated value, and userspace has no indication
that a previous, error-returning write left state behind.

Tighten the entry check from "count > MAX_EXTRADATA_VALUE_LEN" to
"count >= MAX_EXTRADATA_VALUE_LEN". With count strictly less than
sizeof(udm->value), strscpy() can no longer return -E2BIG here, so
the corrupting truncation path is removed entirely.

Fixes: 8a6d5fec6c7f ("net: netconsole: add a userdata config_group member to netconsole_target")
Signed-off-by: Breno Leitao <leitao@debian.org>
---
 drivers/net/netconsole.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

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

-- 
2.52.0

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

[Simon Horman]

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.

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

This isn't a bug introduced by this patch, but does trim_newline() handle
empty strings correctly?

If userspace writes a single NUL byte, strscpy() will successfully copy it and
return 0. Inside trim_newline(), strnlen() would evaluate to 0.

The subsequent check for a newline character would evaluate s[-1]. If the byte
located immediately before the buffer happens to be a newline character, it
would be overwritten with a NUL byte.

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

[Breno Leitao]

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

[PATCH net 3/3] netconsole: propagate device name truncation in dev_name_store()

[Breno Leitao]

dev_name_store() calls strscpy(nt->np.dev_name, buf, IFNAMSIZ) without
checking the return value. If userspace writes an interface name longer
than IFNAMSIZ - 1, strscpy() silently truncates and returns -E2BIG, but
the function ignores it and reports a fully successful write back to
userspace.

If a real interface happens to match the truncated name, netconsole will
bind to the wrong device on the next enable, sending kernel logs and
panic output to an unintended network segment with no indication to
userspace that anything was rewritten.

Reject writes whose length cannot fit in nt->np.dev_name up front:

	if (count >= IFNAMSIZ)
		return -ENAMETOOLONG;

This is not a big deal of a problem, but, it is still the correct
approach.

Fixes: 0bcc1816188e57 ("[NET] netconsole: Support dynamic reconfiguration using configfs")
Signed-off-by: Breno Leitao <leitao@debian.org>
---
 drivers/net/netconsole.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/net/netconsole.c b/drivers/net/netconsole.c
index 4bef003d9df64..3914fb90f9afd 100644
--- a/drivers/net/netconsole.c
+++ b/drivers/net/netconsole.c
@@ -816,6 +816,9 @@ static ssize_t dev_name_store(struct config_item *item, const char *buf,
 {
 	struct netconsole_target *nt = to_target(item);
 
+	if (count >= IFNAMSIZ)
+		return -ENAMETOOLONG;
+
 	dynamic_netconsole_mutex_lock();
 	if (nt->state == STATE_ENABLED) {
 		pr_err("target (%s) is enabled, disable to update parameters\n",

-- 
2.52.0

Re: [PATCH net 3/3] netconsole: propagate device name truncation in dev_name_store()

[Simon Horman]

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: propagate device name truncation in dev_name_store()

This commit rejects interface name writes that exceed the size of the
destination buffer to avoid silently truncating names and binding to
unintended devices.

> diff --git a/drivers/net/netconsole.c b/drivers/net/netconsole.c
> index 4bef003d9df64..3914fb90f9afd 100644
> --- a/drivers/net/netconsole.c
> +++ b/drivers/net/netconsole.c
> @@ -816,6 +816,9 @@ static ssize_t dev_name_store(struct config_item *item, const char *buf,
>  {
>  	struct netconsole_target *nt = to_target(item);
>  
> +	if (count >= IFNAMSIZ)
> +		return -ENAMETOOLONG;
> +

Does this check incorrectly reject valid maximum-length interface names when
written with a trailing newline?

Standard userspace tools like echo append a trailing newline when writing to
configfs attributes. If userspace writes a 15-character name, the count will
be exactly 16 bytes.

Since IFNAMSIZ is 16, the condition count >= IFNAMSIZ evaluates to true,
causing dev_name_store() to reject the valid input with -ENAMETOOLONG.

Prior to this patch, strscpy() safely truncated the trailing newline by
copying the 15 characters and a NUL terminator.

Re: [PATCH net 3/3] netconsole: propagate device name truncation in dev_name_store()

[Breno Leitao]

On Sun, Apr 26, 2026 at 09:39:05AM +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: propagate device name truncation in dev_name_store()
> 
> This commit rejects interface name writes that exceed the size of the
> destination buffer to avoid silently truncating names and binding to
> unintended devices.
> 
> > diff --git a/drivers/net/netconsole.c b/drivers/net/netconsole.c
> > index 4bef003d9df64..3914fb90f9afd 100644
> > --- a/drivers/net/netconsole.c
> > +++ b/drivers/net/netconsole.c
> > @@ -816,6 +816,9 @@ static ssize_t dev_name_store(struct config_item *item, const char *buf,
> >  {
> >  	struct netconsole_target *nt = to_target(item);
> >  
> > +	if (count >= IFNAMSIZ)
> > +		return -ENAMETOOLONG;
> > +
> 
> Does this check incorrectly reject valid maximum-length interface names when
> written with a trailing newline?
> 
> Standard userspace tools like echo append a trailing newline when writing to
> configfs attributes. If userspace writes a 15-character name, the count will
> be exactly 16 bytes.
> 
> Since IFNAMSIZ is 16, the condition count >= IFNAMSIZ evaluates to true,
> causing dev_name_store() to reject the valid input with -ENAMETOOLONG.
> 
> Prior to this patch, strscpy() safely truncated the trailing newline by
> copying the 15 characters and a NUL terminator.

That is a valid issue, if someone is using a 16-byte ifname, it will fail, for
instance:

echo eth0123456789012 (15 chars) writes 16 bytes including the trailing \n

With IFNAMSIZ=16, a valid 15-character interface name written via echo arrives
as 16 bytes (15 chars + \n), and the count >= IFNAMSIZ check rejects it — a
regression compared to the prior strscpy() + trim_newline() behavior, which
silently dropped the newline.

I think a better approach would be:

  size_t len = count;

  if (len && buf[len - 1] == '\n')
      len--;
  if (len >= IFNAMSIZ)
      return -ENAMETOOLONG;

That keeps the length check consistent with what trim_newline() does to the
stored string.

I will send a v2.