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

[Simon Horman <horms@kernel.org>]

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.