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