| Message ID | 20230620152549.2109063-1-Igor.A.Artemiev@mcst.ru (mailing list archive) |
|---|---|
| State | Awaiting Upstream |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [lvc-project] netfilter: ebtables: remove unnecessary NULL check | expand |
Igor Artemiev <Igor.A.Artemiev@mcst.ru> 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?
Igor A. Artemiev <Igor.A.Artemiev@mcst.ru> wrote: > On 6/20/23 19:38, Florian Westphal wrote: > > Igor Artemiev <Igor.A.Artemiev@mcst.ru> 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.
On 6/20/23 19:38, Florian Westphal wrote: > Igor Artemiev <Igor.A.Artemiev@mcst.ru> 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
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);
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 <Igor.A.Artemiev@mcst.ru> --- net/bridge/netfilter/ebtables.c | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-)