| Message ID | 20220820173555.131326-1-fw@strlen.de (mailing list archive) |
|---|---|
| State | Awaiting Upstream |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [nf] netfilter: ebtables: reject blobs that don't provide all entry points | expand |
Hi Florian, On 20/08/22 11:05 pm, Florian Westphal wrote: > For some reason ebtables reject blobs that provide entry points that are > not supported by the table. > > What it should instead reject is the opposite, i.e. rulesets that > DO NOT provide an entry point that is supported by the table. > > t->valid_hooks is the bitmask of hooks (input, forward ...) that will > see packets. So, providing an entry point that is not support is > harmless (never called/used), but the reverse is NOT, this will cause > crash because the ebtables traverser doesn't expect a NULL blob for > a location its receiving packets for. > > Instead of fixing all the individual checks, do what iptables is doing and > reject all blobs that doesn't provide the expected hooks. > > Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2") > Reported-by: Harshit Mogalapalli <harshit.m.mogalapalli@oracle.com> > Signed-off-by: Florian Westphal <fw@strlen.de> > --- > Harshit, can you check if this also silences your reproducer? > Thanks for the patch, I have run the reproducer on patched kernel(this patch) multiple times and the problem is not seen. So it silences the reproducer. Regards, Harshit > Thanks! > > include/linux/netfilter_bridge/ebtables.h | 4 ---- > net/bridge/netfilter/ebtable_broute.c | 8 -------- > net/bridge/netfilter/ebtable_filter.c | 8 -------- > net/bridge/netfilter/ebtable_nat.c | 8 -------- > net/bridge/netfilter/ebtables.c | 8 +------- > 5 files changed, 1 insertion(+), 35 deletions(-) > > diff --git a/include/linux/netfilter_bridge/ebtables.h b/include/linux/netfilter_bridge/ebtables.h > index a13296d6c7ce..fd533552a062 100644 > --- a/include/linux/netfilter_bridge/ebtables.h > +++ b/include/linux/netfilter_bridge/ebtables.h > @@ -94,10 +94,6 @@ struct ebt_table { > struct ebt_replace_kernel *table; > unsigned int valid_hooks; > rwlock_t lock; > - /* e.g. could be the table explicitly only allows certain > - * matches, targets, ... 0 == let it in */ > - int (*check)(const struct ebt_table_info *info, > - unsigned int valid_hooks); > /* the data used by the kernel */ > struct ebt_table_info *private; > struct nf_hook_ops *ops; > diff --git a/net/bridge/netfilter/ebtable_broute.c b/net/bridge/netfilter/ebtable_broute.c > index 1a11064f9990..8f19253024b0 100644 > --- a/net/bridge/netfilter/ebtable_broute.c > +++ b/net/bridge/netfilter/ebtable_broute.c > @@ -36,18 +36,10 @@ static struct ebt_replace_kernel initial_table = { > .entries = (char *)&initial_chain, > }; > > -static int check(const struct ebt_table_info *info, unsigned int valid_hooks) > -{ > - if (valid_hooks & ~(1 << NF_BR_BROUTING)) > - return -EINVAL; > - return 0; > -} > - > static const struct ebt_table broute_table = { > .name = "broute", > .table = &initial_table, > .valid_hooks = 1 << NF_BR_BROUTING, > - .check = check, > .me = THIS_MODULE, > }; > > diff --git a/net/bridge/netfilter/ebtable_filter.c b/net/bridge/netfilter/ebtable_filter.c > index cb949436bc0e..278f324e6752 100644 > --- a/net/bridge/netfilter/ebtable_filter.c > +++ b/net/bridge/netfilter/ebtable_filter.c > @@ -43,18 +43,10 @@ static struct ebt_replace_kernel initial_table = { > .entries = (char *)initial_chains, > }; > > -static int check(const struct ebt_table_info *info, unsigned int valid_hooks) > -{ > - if (valid_hooks & ~FILTER_VALID_HOOKS) > - return -EINVAL; > - return 0; > -} > - > static const struct ebt_table frame_filter = { > .name = "filter", > .table = &initial_table, > .valid_hooks = FILTER_VALID_HOOKS, > - .check = check, > .me = THIS_MODULE, > }; > > diff --git a/net/bridge/netfilter/ebtable_nat.c b/net/bridge/netfilter/ebtable_nat.c > index 5ee0531ae506..9066f7f376d5 100644 > --- a/net/bridge/netfilter/ebtable_nat.c > +++ b/net/bridge/netfilter/ebtable_nat.c > @@ -43,18 +43,10 @@ static struct ebt_replace_kernel initial_table = { > .entries = (char *)initial_chains, > }; > > -static int check(const struct ebt_table_info *info, unsigned int valid_hooks) > -{ > - if (valid_hooks & ~NAT_VALID_HOOKS) > - return -EINVAL; > - return 0; > -} > - > static const struct ebt_table frame_nat = { > .name = "nat", > .table = &initial_table, > .valid_hooks = NAT_VALID_HOOKS, > - .check = check, > .me = THIS_MODULE, > }; > > diff --git a/net/bridge/netfilter/ebtables.c b/net/bridge/netfilter/ebtables.c > index f2dbefb61ce8..9a0ae59cdc50 100644 > --- a/net/bridge/netfilter/ebtables.c > +++ b/net/bridge/netfilter/ebtables.c > @@ -1040,8 +1040,7 @@ static int do_replace_finish(struct net *net, struct ebt_replace *repl, > goto free_iterate; > } > > - /* the table doesn't like it */ > - if (t->check && (ret = t->check(newinfo, repl->valid_hooks))) > + if (repl->valid_hooks != t->valid_hooks) > goto free_unlock; > > if (repl->num_counters && repl->num_counters != t->private->nentries) { > @@ -1231,11 +1230,6 @@ int ebt_register_table(struct net *net, const struct ebt_table *input_table, > if (ret != 0) > goto free_chainstack; > > - if (table->check && table->check(newinfo, table->valid_hooks)) { > - ret = -EINVAL; > - goto free_chainstack; > - } > - > table->private = newinfo; > rwlock_init(&table->lock); > mutex_lock(&ebt_mutex);
On 8/20/22 12:35 PM, Florian Westphal wrote: > For some reason ebtables reject blobs that provide entry points that are > not supported by the table. > > What it should instead reject is the opposite, i.e. rulesets that > DO NOT provide an entry point that is supported by the table. > > t->valid_hooks is the bitmask of hooks (input, forward ...) that will > see packets. So, providing an entry point that is not support is > harmless (never called/used), but the reverse is NOT, this will cause > crash because the ebtables traverser doesn't expect a NULL blob for > a location its receiving packets for. > > Instead of fixing all the individual checks, do what iptables is doing and > reject all blobs that doesn't provide the expected hooks. > > Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2") > Reported-by: Harshit Mogalapalli <harshit.m.mogalapalli@oracle.com> > Signed-off-by: Florian Westphal <fw@strlen.de> Hi, Could you please add the panic stack mentioned above and syzkaller reproducer ID to the commit text ? > --- > Harshit, can you check if this also silences your reproducer? > > Thanks! > > include/linux/netfilter_bridge/ebtables.h | 4 ---- > net/bridge/netfilter/ebtable_broute.c | 8 -------- > net/bridge/netfilter/ebtable_filter.c | 8 -------- > net/bridge/netfilter/ebtable_nat.c | 8 -------- > net/bridge/netfilter/ebtables.c | 8 +------- > 5 files changed, 1 insertion(+), 35 deletions(-) > > diff --git a/include/linux/netfilter_bridge/ebtables.h b/include/linux/netfilter_bridge/ebtables.h > index a13296d6c7ce..fd533552a062 100644 > --- a/include/linux/netfilter_bridge/ebtables.h > +++ b/include/linux/netfilter_bridge/ebtables.h > @@ -94,10 +94,6 @@ struct ebt_table { > struct ebt_replace_kernel *table; > unsigned int valid_hooks; > rwlock_t lock; > - /* e.g. could be the table explicitly only allows certain > - * matches, targets, ... 0 == let it in */ > - int (*check)(const struct ebt_table_info *info, > - unsigned int valid_hooks); > /* the data used by the kernel */ > struct ebt_table_info *private; > struct nf_hook_ops *ops; > diff --git a/net/bridge/netfilter/ebtable_broute.c b/net/bridge/netfilter/ebtable_broute.c > index 1a11064f9990..8f19253024b0 100644 > --- a/net/bridge/netfilter/ebtable_broute.c > +++ b/net/bridge/netfilter/ebtable_broute.c > @@ -36,18 +36,10 @@ static struct ebt_replace_kernel initial_table = { > .entries = (char *)&initial_chain, > }; > > -static int check(const struct ebt_table_info *info, unsigned int valid_hooks) > -{ > - if (valid_hooks & ~(1 << NF_BR_BROUTING)) > - return -EINVAL; > - return 0; > -} > - > static const struct ebt_table broute_table = { > .name = "broute", > .table = &initial_table, > .valid_hooks = 1 << NF_BR_BROUTING, > - .check = check, > .me = THIS_MODULE, > }; > > diff --git a/net/bridge/netfilter/ebtable_filter.c b/net/bridge/netfilter/ebtable_filter.c > index cb949436bc0e..278f324e6752 100644 > --- a/net/bridge/netfilter/ebtable_filter.c > +++ b/net/bridge/netfilter/ebtable_filter.c > @@ -43,18 +43,10 @@ static struct ebt_replace_kernel initial_table = { > .entries = (char *)initial_chains, > }; > > -static int check(const struct ebt_table_info *info, unsigned int valid_hooks) > -{ > - if (valid_hooks & ~FILTER_VALID_HOOKS) > - return -EINVAL; > - return 0; > -} > - > static const struct ebt_table frame_filter = { > .name = "filter", > .table = &initial_table, > .valid_hooks = FILTER_VALID_HOOKS, > - .check = check, > .me = THIS_MODULE, > }; > > diff --git a/net/bridge/netfilter/ebtable_nat.c b/net/bridge/netfilter/ebtable_nat.c > index 5ee0531ae506..9066f7f376d5 100644 > --- a/net/bridge/netfilter/ebtable_nat.c > +++ b/net/bridge/netfilter/ebtable_nat.c > @@ -43,18 +43,10 @@ static struct ebt_replace_kernel initial_table = { > .entries = (char *)initial_chains, > }; > > -static int check(const struct ebt_table_info *info, unsigned int valid_hooks) > -{ > - if (valid_hooks & ~NAT_VALID_HOOKS) > - return -EINVAL; > - return 0; > -} > - > static const struct ebt_table frame_nat = { > .name = "nat", > .table = &initial_table, > .valid_hooks = NAT_VALID_HOOKS, > - .check = check, > .me = THIS_MODULE, > }; > > diff --git a/net/bridge/netfilter/ebtables.c b/net/bridge/netfilter/ebtables.c > index f2dbefb61ce8..9a0ae59cdc50 100644 > --- a/net/bridge/netfilter/ebtables.c > +++ b/net/bridge/netfilter/ebtables.c > @@ -1040,8 +1040,7 @@ static int do_replace_finish(struct net *net, struct ebt_replace *repl, > goto free_iterate; > } > > - /* the table doesn't like it */ > - if (t->check && (ret = t->check(newinfo, repl->valid_hooks))) > + if (repl->valid_hooks != t->valid_hooks) > goto free_unlock; > > if (repl->num_counters && repl->num_counters != t->private->nentries) { > @@ -1231,11 +1230,6 @@ int ebt_register_table(struct net *net, const struct ebt_table *input_table, > if (ret != 0) > goto free_chainstack; > > - if (table->check && table->check(newinfo, table->valid_hooks)) { > - ret = -EINVAL; > - goto free_chainstack; > - } > - > table->private = newinfo; > rwlock_init(&table->lock); > mutex_lock(&ebt_mutex);
john.p.donnelly@oracle.com <john.p.donnelly@oracle.com> wrote: > On 8/20/22 12:35 PM, Florian Westphal wrote: > > For some reason ebtables reject blobs that provide entry points that are > > not supported by the table. > > > > What it should instead reject is the opposite, i.e. rulesets that > > DO NOT provide an entry point that is supported by the table. > > > > t->valid_hooks is the bitmask of hooks (input, forward ...) that will > > see packets. So, providing an entry point that is not support is > > harmless (never called/used), but the reverse is NOT, this will cause > > crash because the ebtables traverser doesn't expect a NULL blob for > > a location its receiving packets for. > > > > Instead of fixing all the individual checks, do what iptables is doing and > > reject all blobs that doesn't provide the expected hooks. > > > > Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2") > > Reported-by: Harshit Mogalapalli <harshit.m.mogalapalli@oracle.com> > > Signed-off-by: Florian Westphal <fw@strlen.de> > > Hi, > > Could you please add the panic stack mentioned above and syzkaller > reproducer ID to the commit text ? I did not see a reproducer ID. What ended up in the tree is this: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=7997eff82828304b780dc0a39707e1946d6f1ebf
On 8/29/22 9:03 AM, Florian Westphal wrote: > john.p.donnelly@oracle.com <john.p.donnelly@oracle.com> wrote: >> On 8/20/22 12:35 PM, Florian Westphal wrote: >>> For some reason ebtables reject blobs that provide entry points that are >>> not supported by the table. >>> >>> What it should instead reject is the opposite, i.e. rulesets that >>> DO NOT provide an entry point that is supported by the table. >>> >>> t->valid_hooks is the bitmask of hooks (input, forward ...) that will >>> see packets. So, providing an entry point that is not support is >>> harmless (never called/used), but the reverse is NOT, this will cause >>> crash because the ebtables traverser doesn't expect a NULL blob for >>> a location its receiving packets for. >>> >>> Instead of fixing all the individual checks, do what iptables is doing and >>> reject all blobs that doesn't provide the expected hooks. >>> >>> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2") >>> Reported-by: Harshit Mogalapalli <harshit.m.mogalapalli@oracle.com> >>> Signed-off-by: Florian Westphal <fw@strlen.de> >> >> Hi, >> >> Could you please add the panic stack mentioned above and syzkaller >> reproducer ID to the commit text ? > > I did not see a reproducer ID. What ended up in the tree is this: > > https://urldefense.com/v3/__https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=7997eff82828304b780dc0a39707e1946d6f1ebf__;!!ACWV5N9M2RV99hQ!JxonjgQUi7Mbcd-ouxRwPgu8Jwl6ej2rO4pTvYMtteWexclV5-hciu9e5rgtkXoB7dyAdLCyZ4EQ9HQj$ Thank you !
diff --git a/include/linux/netfilter_bridge/ebtables.h b/include/linux/netfilter_bridge/ebtables.h index a13296d6c7ce..fd533552a062 100644 --- a/include/linux/netfilter_bridge/ebtables.h +++ b/include/linux/netfilter_bridge/ebtables.h @@ -94,10 +94,6 @@ struct ebt_table { struct ebt_replace_kernel *table; unsigned int valid_hooks; rwlock_t lock; - /* e.g. could be the table explicitly only allows certain - * matches, targets, ... 0 == let it in */ - int (*check)(const struct ebt_table_info *info, - unsigned int valid_hooks); /* the data used by the kernel */ struct ebt_table_info *private; struct nf_hook_ops *ops; diff --git a/net/bridge/netfilter/ebtable_broute.c b/net/bridge/netfilter/ebtable_broute.c index 1a11064f9990..8f19253024b0 100644 --- a/net/bridge/netfilter/ebtable_broute.c +++ b/net/bridge/netfilter/ebtable_broute.c @@ -36,18 +36,10 @@ static struct ebt_replace_kernel initial_table = { .entries = (char *)&initial_chain, }; -static int check(const struct ebt_table_info *info, unsigned int valid_hooks) -{ - if (valid_hooks & ~(1 << NF_BR_BROUTING)) - return -EINVAL; - return 0; -} - static const struct ebt_table broute_table = { .name = "broute", .table = &initial_table, .valid_hooks = 1 << NF_BR_BROUTING, - .check = check, .me = THIS_MODULE, }; diff --git a/net/bridge/netfilter/ebtable_filter.c b/net/bridge/netfilter/ebtable_filter.c index cb949436bc0e..278f324e6752 100644 --- a/net/bridge/netfilter/ebtable_filter.c +++ b/net/bridge/netfilter/ebtable_filter.c @@ -43,18 +43,10 @@ static struct ebt_replace_kernel initial_table = { .entries = (char *)initial_chains, }; -static int check(const struct ebt_table_info *info, unsigned int valid_hooks) -{ - if (valid_hooks & ~FILTER_VALID_HOOKS) - return -EINVAL; - return 0; -} - static const struct ebt_table frame_filter = { .name = "filter", .table = &initial_table, .valid_hooks = FILTER_VALID_HOOKS, - .check = check, .me = THIS_MODULE, }; diff --git a/net/bridge/netfilter/ebtable_nat.c b/net/bridge/netfilter/ebtable_nat.c index 5ee0531ae506..9066f7f376d5 100644 --- a/net/bridge/netfilter/ebtable_nat.c +++ b/net/bridge/netfilter/ebtable_nat.c @@ -43,18 +43,10 @@ static struct ebt_replace_kernel initial_table = { .entries = (char *)initial_chains, }; -static int check(const struct ebt_table_info *info, unsigned int valid_hooks) -{ - if (valid_hooks & ~NAT_VALID_HOOKS) - return -EINVAL; - return 0; -} - static const struct ebt_table frame_nat = { .name = "nat", .table = &initial_table, .valid_hooks = NAT_VALID_HOOKS, - .check = check, .me = THIS_MODULE, }; diff --git a/net/bridge/netfilter/ebtables.c b/net/bridge/netfilter/ebtables.c index f2dbefb61ce8..9a0ae59cdc50 100644 --- a/net/bridge/netfilter/ebtables.c +++ b/net/bridge/netfilter/ebtables.c @@ -1040,8 +1040,7 @@ static int do_replace_finish(struct net *net, struct ebt_replace *repl, goto free_iterate; } - /* the table doesn't like it */ - if (t->check && (ret = t->check(newinfo, repl->valid_hooks))) + if (repl->valid_hooks != t->valid_hooks) goto free_unlock; if (repl->num_counters && repl->num_counters != t->private->nentries) { @@ -1231,11 +1230,6 @@ int ebt_register_table(struct net *net, const struct ebt_table *input_table, if (ret != 0) goto free_chainstack; - if (table->check && table->check(newinfo, table->valid_hooks)) { - ret = -EINVAL; - goto free_chainstack; - } - table->private = newinfo; rwlock_init(&table->lock); mutex_lock(&ebt_mutex);
For some reason ebtables reject blobs that provide entry points that are not supported by the table. What it should instead reject is the opposite, i.e. rulesets that DO NOT provide an entry point that is supported by the table. t->valid_hooks is the bitmask of hooks (input, forward ...) that will see packets. So, providing an entry point that is not support is harmless (never called/used), but the reverse is NOT, this will cause crash because the ebtables traverser doesn't expect a NULL blob for a location its receiving packets for. Instead of fixing all the individual checks, do what iptables is doing and reject all blobs that doesn't provide the expected hooks. Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2") Reported-by: Harshit Mogalapalli <harshit.m.mogalapalli@oracle.com> Signed-off-by: Florian Westphal <fw@strlen.de> --- Harshit, can you check if this also silences your reproducer? Thanks! include/linux/netfilter_bridge/ebtables.h | 4 ---- net/bridge/netfilter/ebtable_broute.c | 8 -------- net/bridge/netfilter/ebtable_filter.c | 8 -------- net/bridge/netfilter/ebtable_nat.c | 8 -------- net/bridge/netfilter/ebtables.c | 8 +------- 5 files changed, 1 insertion(+), 35 deletions(-)