2023-06-20 15:50:29

by Igor Artemiev

Subject: [lvc-project] [PATCH] netfilter: ebtables: remove unnecessary NULL check

In ebt_do_table() 'private->chainstack' cannot be NULL
and the 'cs' pointer is dereferenced below, so it does not make
sense to compare 'private->chainstack' with NULL.

Found by Linux Verification Center (linuxtesting.org) with SVACE.

Signed-off-by: Igor Artemiev <[email protected]>
---
net/bridge/netfilter/ebtables.c | 5 +----
1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/net/bridge/netfilter/ebtables.c b/net/bridge/netfilter/ebtables.c
index 757ec46fc45a..74daca8a5142 100644
--- a/net/bridge/netfilter/ebtables.c
+++ b/net/bridge/netfilter/ebtables.c
@@ -212,10 +212,7 @@ unsigned int ebt_do_table(void *priv, struct sk_buff *skb,
private = table->private;
cb_base = COUNTER_BASE(private->counters, private->nentries,
smp_processor_id());
- if (private->chainstack)
- cs = private->chainstack[smp_processor_id()];
- else
- cs = NULL;
+ cs = private->chainstack[smp_processor_id()];
chaininfo = private->hook_entry[hook];
nentries = private->hook_entry[hook]->nentries;
point = (struct ebt_entry *)(private->hook_entry[hook]->data);
--
2.30.2

2023-06-20 17:04:10

by Florian Westphal

Subject: Re: [lvc-project] [PATCH] netfilter: ebtables: remove unnecessary NULL check

Igor Artemiev <[email protected]> wrote:
> In ebt_do_table() 'private->chainstack' cannot be NULL
> and the 'cs' pointer is dereferenced below, so it does not make
> sense to compare 'private->chainstack' with NULL.

? Why do you think that?

> + cs = private->chainstack[smp_processor_id()];

Looks like NULL deref to me. Did you test this?

2023-06-21 10:45:16

by Igor Artemiev

Subject: Re: [lvc-project] [PATCH] netfilter: ebtables: remove unnecessary NULL check

On 6/20/23 19:38, Florian Westphal wrote:
> Igor Artemiev <[email protected]> wrote:
>> In ebt_do_table() 'private->chainstack' cannot be NULL
>> and the 'cs' pointer is dereferenced below, so it does not make
>> sense to compare 'private->chainstack' with NULL.
> ? Why do you think that?
>
The 'cs' pointer is dereferenced below without checking, as it is
assumed to always be initialized with
'private->chainstack[smp_processor_id()]'.
>> + cs = private->chainstack[smp_processor_id()];
> Looks like NULL deref to me. Did you test this?
>
No, I didn't test this.

Thanks,
Igor

2023-06-21 11:39:00

by Florian Westphal

Subject: Re: [lvc-project] [PATCH] netfilter: ebtables: remove unnecessary NULL check

Igor A. Artemiev <[email protected]> wrote:
> On 6/20/23 19:38, Florian Westphal wrote:
> > Igor Artemiev <[email protected]> wrote:
> > > In ebt_do_table() 'private->chainstack' cannot be NULL
> > > and the 'cs' pointer is dereferenced below, so it does not make
> > > sense to compare 'private->chainstack' with NULL.
> > ? Why do you think that?
> >
> The 'cs' pointer is dereferenced below without checking, as it is assumed to
> always be initialized with 'private->chainstack[smp_processor_id()]'.

No, its not. The dereferencing is conditional, as is the allocation
of the chainstack.

No user defined chains, no chain stack.

With this change, "ebtables-legacy -A INPUT" causes kernel panic.