net: sched: em_text: require NUL-terminated algo name
Subsystems (3)
PATCH 1/1 net: sched: em_text: require NUL-terminated algo name
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.
---
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(+)
net/sched/em_text.c +3 -0
@@ -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 ; +if (conf->from_layer > conf->to_layer)return -EINVAL ;
Signature · 1 line
2.53.0
Paolo Abeni
maintainerQuoted 38 lines · Greg Kroah-Hartman
em_text_change()copies the user-suppliedtcf_em_textstruct from netlink and passesconf->algostraight totextsearch_prepare(), which forwards it tolookup_ts_algo()(strcmp) andrequest_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()forxt_stringnetfilter matching function, but for some reason was not added here, so fix this up by doing the same exact thing.Cc Jamal Hadi Salim <[email protected]>Cc Jiri Pirko <[email protected]>Cc "David S. Miller" <[email protected]>Cc Eric Dumazet <[email protected]>Cc Jakub Kicinski <[email protected]>Cc Paolo Abeni <[email protected]>Cc Simon Horman <[email protected]>Fixes d675c989ed2d ("[PKT_SCHED]: Packet classification based on textsearch (ematch)")Cc stable <[email protected]>Assisted-by gregkh_clanker_t1000Signed-off-by Greg Kroah-Hartman <[email protected]>---
Note, my tools flagged this, so I fixed this up the same way thatstring_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(+)net/sched/em_text.c +0
-0
net/sched/em_text.c static int em_text_change(struct net *net, void *data, int len, · line 62
Quoted 5 lines
net/sched/em_text.c +2
-0@@ -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