Re: [PATCH net] net: sched: em_text: require NUL-terminated algo name

[Paolo Abeni <pabeni@redhat.com>]

On 4/11/26 1:10 PM, Greg Kroah-Hartman wrote:
> em_text_change() copies the user-supplied tcf_em_text struct from
> netlink and passes conf->algo straight to textsearch_prepare(), which
> forwards it to lookup_ts_algo() (strcmp) and request_module() (vsnprintf
> %s).  But the algo[16] field is never validated to be NULL-terminated,
> so a fully populated array reads past it into the adjacent
> from_offset/to_offset/pattern_len fields and the trailing pattern bytes
> during the string operations.
> 
> This type of pattern is properly checked in the string_mt_check() for
> xt_string netfilter matching function, but for some reason was not added
> here, so fix this up by doing the same exact thing.
> 
> Cc: Jamal Hadi Salim <jhs@mojatatu.com>
> Cc: Jiri Pirko <jiri@resnulli.us>
> Cc: "David S. Miller" <davem@davemloft.net>
> Cc: Eric Dumazet <edumazet@google.com>
> Cc: Jakub Kicinski <kuba@kernel.org>
> Cc: Paolo Abeni <pabeni@redhat.com>
> Cc: Simon Horman <horms@kernel.org>
> Fixes: d675c989ed2d ("[PKT_SCHED]: Packet classification based on textsearch (ematch)")
> Cc: stable <stable@kernel.org>
> Assisted-by: gregkh_clanker_t1000
> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> ---
> Note, my tools flagged this, so I fixed this up the same way that
> string_mt_check() did, but if there is some other way that this should
> be resolved, or I got this totally wrong that this isn't an issue,
> please let me know, thanks!
> 
> 
>  net/sched/em_text.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/net/sched/em_text.c b/net/sched/em_text.c
> index 343f1aebeec2..24a8aa21971d 100644
> --- a/net/sched/em_text.c
> +++ b/net/sched/em_text.c
> @@ -58,6 +58,9 @@ static int em_text_change(struct net *net, void *data, int len,
>  	if (len < sizeof(*conf) || len < (sizeof(*conf) + conf->pattern_len))
>  		return -EINVAL;
>  
> +	if (conf->algo[sizeof(conf->algo) - 1] != '\0')
> +		return -EINVAL;

The addressed issue looks real to me, but sashiko found a problem with
this fix:

---
Is this NUL-termination check too strict and could it introduce an ABI
regression?
If a userspace application sends a valid, NUL-terminated string that is
shorter than 15 characters, but does not explicitly zero-pad the
remaining uninitialized bytes in the tcf_em_text struct, this check will
fail with -EINVAL.
Prior to this patch, the kernel's string operations safely stopped
reading at the first \0 byte, correctly processing these shorter
strings. Rejecting previously valid structures breaks backward
compatibility with existing userspace binaries.
Would it be better to check for NUL-termination using memchr() or
strnlen() instead? For example, memchr(conf->algo, '\0',
sizeof(conf->algo)) == NULL.
---

/P