[PATCH net] net/sched: cls_api: fix error handling causing NULL dereference

[PATCH net] net/sched: cls_api: fix error handling causing NULL dereference

[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 <pierre@stackhpc.com>
---
 net/sched/cls_api.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
index 8e47e5355be6..4f648af8cfaa 100644
--- a/net/sched/cls_api.c
+++ b/net/sched/cls_api.c
@@ -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;
-- 
2.43.5

Re: [PATCH net] net/sched: cls_api: fix error handling causing NULL dereference

[Michal Swiatkowski]

On Thu, Feb 13, 2025 at 11:36:10PM +0100, Pierre Riteau wrote:
> 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 <pierre@stackhpc.com>
> ---
>  net/sched/cls_api.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
> index 8e47e5355be6..4f648af8cfaa 100644
> --- a/net/sched/cls_api.c
> +++ b/net/sched/cls_api.c
> @@ -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 <michal.swiatkowski@linux.intel.com>

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

> -- 
> 2.43.5

Re: [PATCH net] net/sched: cls_api: fix error handling causing NULL dereference

[Pierre Riteau]

On Fri, 14 Feb 2025 at 13:46, Michal Swiatkowski
<michal.swiatkowski@linux.intel.com> wrote:
>
> On Thu, Feb 13, 2025 at 11:36:10PM +0100, Pierre Riteau wrote:
> > 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 <pierre@stackhpc.com>
> > ---
> >  net/sched/cls_api.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
> > index 8e47e5355be6..4f648af8cfaa 100644
> > --- a/net/sched/cls_api.c
> > +++ b/net/sched/cls_api.c
> > @@ -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 <michal.swiatkowski@linux.intel.com>
>
> 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?

Re: [PATCH net] net/sched: cls_api: fix error handling causing NULL dereference

[Pierre Riteau]

On Fri, 14 Feb 2025 at 15:21, Pierre Riteau <pierre@stackhpc.com> wrote:
>
> On Fri, 14 Feb 2025 at 13:46, Michal Swiatkowski
> <michal.swiatkowski@linux.intel.com> wrote:
> >
> > On Thu, Feb 13, 2025 at 11:36:10PM +0100, Pierre Riteau wrote:
> > > 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 <pierre@stackhpc.com>
> > > ---
> > >  net/sched/cls_api.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
> > > index 8e47e5355be6..4f648af8cfaa 100644
> > > --- a/net/sched/cls_api.c
> > > +++ b/net/sched/cls_api.c
> > > @@ -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 <michal.swiatkowski@linux.intel.com>
> >
> > 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?

Nevermind, I see you already posted a patch in the meantime. Thanks.

Re: [PATCH net] net/sched: cls_api: fix error handling causing NULL dereference

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Thu, 13 Feb 2025 23:36:10 +0100 you wrote:
> 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.
> 
> [...]

Here is the summary with links:
  - [net] net/sched: cls_api: fix error handling causing NULL dereference
    https://git.kernel.org/netdev/net/c/071ed42cff4f

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html