net/sched: cls_api: fix error handling causing NULL dereference

tcf_exts_miss_cookie_base_alloc() calls xa_alloc_cyclic() which can return 1 if the allocation succeeded after wrapping. This was treated as an error, with value 1 returned to caller tcf_exts_init_ex() which sets exts->actions to NULL and returns 1 to caller fl_change().

fl_change() treats err == 1 as success, calling tcf_exts_validate_ex() which calls tcf_action_init() with exts->actions as argument, where it is dereferenced.

Example trace:

BUG: kernel NULL pointer dereference, address: 0000000000000000
CPU: 114 PID: 16151 Comm: handler114 Kdump: loaded Not tainted 5.14.0-503.16.1.el9_5.x86_64 #1
RIP: 0010:tcf_action_init+0x1f8/0x2c0
Call Trace:
 tcf_action_init+0x1f8/0x2c0
 tcf_exts_validate_ex+0x175/0x190
 fl_change+0x537/0x1120 [cls_flower]

Fixes 80cd22c35c90 ("net/sched: cls_api: Support hardware miss to tc action")

Signed-off-by Pierre Riteau <[email protected]>

---
 net/sched/cls_api.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

net/sched/cls_api.c +0 -0

index 8e47e5355be6..4f648af8cfaa 100644

Signature · 1 line

2.43.5

Quoted 30 lines · Pierre Riteau

tcf_exts_miss_cookie_base_alloc() calls xa_alloc_cyclic() which can return 1 if the allocation succeeded after wrapping. This was treated as an error, with value 1 returned to caller tcf_exts_init_ex() which sets exts->actions to NULL and returns 1 to caller fl_change().

fl_change() treats err == 1 as success, calling tcf_exts_validate_ex() which calls tcf_action_init() with exts->actions as argument, where it is dereferenced.

Example trace:

BUG: kernel NULL pointer dereference, address: 0000000000000000
CPU: 114 PID: 16151 Comm: handler114 Kdump: loaded Not tainted 5.14.0-503.16.1.el9_5.x86_64 #1
RIP: 0010:tcf_action_init+0x1f8/0x2c0
Call Trace:
 tcf_action_init+0x1f8/0x2c0
 tcf_exts_validate_ex+0x175/0x190
 fl_change+0x537/0x1120 [cls_flower]

Fixes 80cd22c35c90 ("net/sched: cls_api: Support hardware miss to tc action")

Signed-off-by Pierre Riteau <[email protected]>

---
 net/sched/cls_api.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

net/sched/cls_api.c +0 -0

index 8e47e5355be6..4f648af8cfaa 100644

net/sched/cls_api.c tcf_exts_miss_cookie_base_alloc(struct tcf_exts *exts, struct tcf_proto *tp, · line 103

Quoted 8 lines

net/sched/cls_api.c +1 -1

@@ -97,7 +97,7 @@ tcf_exts_miss_cookie_base_alloc(struct tcf_exts *exts, struct tcf_proto *tp,
 
      err = xa_alloc_cyclic(&tcf_exts_miss_cookies_xa, &n->miss_cookie_base,
                            n, xa_limit_32b, &next, GFP_KERNEL);
-     if (err)
+     if (err < 0)
              goto err_xa_alloc;
 
      exts->miss_cookie_node = n;

Thanks for fixing.

Reviewed-by Michal Swiatkowski <[email protected]>

The same thing is done in devlink_rel_alloc() (net/devlink/core.c). I am not sure if it can lead to NULL pointer dereference as here.

Thanks,
Michal

Quoted 42 lines · Michal Swiatkowski

Quoted 30 lines · Pierre Riteau

tcf_exts_miss_cookie_base_alloc() calls xa_alloc_cyclic() which can return 1 if the allocation succeeded after wrapping. This was treated as an error, with value 1 returned to caller tcf_exts_init_ex() which sets exts->actions to NULL and returns 1 to caller fl_change().

fl_change() treats err == 1 as success, calling tcf_exts_validate_ex() which calls tcf_action_init() with exts->actions as argument, where it is dereferenced.

Example trace:

BUG: kernel NULL pointer dereference, address: 0000000000000000
CPU: 114 PID: 16151 Comm: handler114 Kdump: loaded Not tainted 5.14.0-503.16.1.el9_5.x86_64 #1
RIP: 0010:tcf_action_init+0x1f8/0x2c0
Call Trace:
 tcf_action_init+0x1f8/0x2c0
 tcf_exts_validate_ex+0x175/0x190
 fl_change+0x537/0x1120 [cls_flower]

Fixes 80cd22c35c90 ("net/sched: cls_api: Support hardware miss to tc action")

Signed-off-by Pierre Riteau <[email protected]>

---
 net/sched/cls_api.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

net/sched/cls_api.c +0 -0

index 8e47e5355be6..4f648af8cfaa 100644

net/sched/cls_api.c tcf_exts_miss_cookie_base_alloc(struct tcf_exts *exts, struct tcf_proto *tp, · line 103

Quoted 8 lines

net/sched/cls_api.c +1 -1

@@ -97,7 +97,7 @@ tcf_exts_miss_cookie_base_alloc(struct tcf_exts *exts, struct tcf_proto *tp,
 
      err = xa_alloc_cyclic(&tcf_exts_miss_cookies_xa, &n->miss_cookie_base,
                            n, xa_limit_32b, &next, GFP_KERNEL);
-     if (err)
+     if (err < 0)
              goto err_xa_alloc;
 
      exts->miss_cookie_node = n;

Thanks for fixing.

Quoted 7 lines

Reviewed-by Michal Swiatkowski <[email protected]>

The same thing is done in devlink_rel_alloc() (net/devlink/core.c). I am not sure if it can lead to NULL pointer dereference as here.

Thanks,
Michal

Thanks for the review. I also checked other occurrences under net/ and wanted to investigate this one. It looks like it could produce a similar result.

In devlink_rel_alloc(), if xa_alloc_cyclic() returns 1, we execute kfree(rel); return ERR_PTR(err);
In caller devlink_rel_nested_in_add(), we would assign the value 1 to rel, check IS_ERR(rel) - which should be false? - and dereference rel
at: rel->devlink_index = devlink->index;

Should I update the patch to cover both issues?