In ebt_do_table() function dereferencing 'private->hook_entry[hook]'
can lead to NULL pointer dereference. So add a check to prevent that.
Kernel panic:
[ 119.229105][ T31] general protection fault, probably for
non-canonical address 0xdffffc0000000005: 0000 [#1] PREEMPT SMP KASAN
[ 119.230280][ T31] KASAN: null-ptr-deref in range
[0x0000000000000028-0x000000000000002f]
[ 119.231043][ T31] CPU: 3 PID: 31 Comm: kworker/3:0 Not tainted
6.0.0-rc1 #1
[ 119.231652][ T31] Hardware name: QEMU Standard PC (i440FX + PIIX,
1996), BIOS 1.11.0-2.el7 04/01/2014
[ 119.232440][ T31] Workqueue: mld mld_ifc_work
[ 119.232846][ T31] RIP: 0010:ebt_do_table+0x1dc/0x1ce0
[ 119.233291][ T31] Code: 89 fa 48 c1 ea 03 80 3c 02 00 0f 85 5c 16
00 00 48 b8 00 00 00 00 00 fc ff df 49 8b 6c df 08 48 8d 7d 2c 48 89 fa
48 c1 ea 03 <0f> b6 14 02 48 89 f8 83 e0 07 83 c0 03 38 d0 7c 08 84 d2
0f 85 88
[ 119.234920][ T31] RSP: 0018:ffffc90000347178 EFLAGS: 00010217
[ 119.235425][ T31] RAX: dffffc0000000000 RBX: 0000000000000003 RCX:
ffffffff8158677b
[ 119.236097][ T31] RDX: 0000000000000005 RSI: ffffffff889a7b80 RDI:
000000000000002c
[ 119.236764][ T31] RBP: 0000000000000000 R08: 0000000000000001 R09:
ffff888101a78443
[ 119.237425][ T31] R10: ffffed102034f088 R11: 000200100040dd86 R12:
ffffc90001111018
[ 119.238100][ T31] R13: ffffc90001103080 R14: ffffc90001111000 R15:
ffffc90001103000
[ 119.238769][ T31] FS: 0000000000000000(0000)
GS:ffff88811a380000(0000) knlGS:0000000000000000
[ 119.239592][ T31] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 119.240221][ T31] CR2: 00007f6a9adda000 CR3: 0000000100be3000 CR4:
00000000000006e0
[ 119.240970][ T31] Call Trace:
[ 119.241253][ T31] <TASK>
[ 119.241495][ T31] ? ip6_output+0x1f4/0x6e0
[ 119.241877][ T31] ? NF_HOOK.constprop.0+0xe6/0x5f0
[ 119.242309][ T31] ? mld_ifc_work+0x564/0xaa0
[ 119.242708][ T31] ? kthread+0x297/0x340
[ 119.243060][ T31] ? ret_from_fork+0x22/0x30
[ 119.243454][ T31] ? br_multicast_count+0xbf/0x8d0
[ 119.243896][ T31] ? rcu_read_lock_sched_held+0xd/0xa0
[ 119.244356][ T31] ? compat_copy_ebt_replace_from_user+0x380/0x380
[ 119.244907][ T31] ? compat_copy_ebt_replace_from_user+0x380/0x380
[ 119.245454][ T31] nf_hook_slow+0xb1/0x170
[ 119.245835][ T31] __br_forward+0x289/0x730
[ 119.246219][ T31] ? br_forward_finish+0x320/0x320
[ 119.246656][ T31] ? br_dev_queue_push_xmit+0x650/0x650
[ 119.247118][ T31] ? memcpy+0x39/0x60
[ 119.247444][ T31] ? __skb_clone+0x56c/0x750
[ 119.247845][ T31] maybe_deliver+0x24b/0x380
[ 119.248234][ T31] br_flood+0xc6/0x390
[ 119.248577][ T31] br_dev_xmit+0xa2e/0x12c0
[ 119.248975][ T31] ? rcu_read_lock_sched_held+0xd/0xa0
[ 119.249498][ T31] ? br_netpoll_setup+0x170/0x170
[ 119.249987][ T31] ? fib_rules_lookup+0x2eb/0x9d0
[ 119.250462][ T31] ? lock_repin_lock+0x30/0x420
[ 119.250922][ T31] ? rcu_read_lock_sched_held+0xd/0xa0
[ 119.251376][ T31] ? lock_acquire+0x510/0x630
[ 119.251773][ T31] ? netif_inherit_tso_max+0x1e0/0x1e0
[ 119.252228][ T31] dev_hard_start_xmit+0x151/0x660
[ 119.252663][ T31] __dev_queue_xmit+0x240e/0x3200
[ 119.253080][ T31] ? ip6mr_rtm_dumproute+0x530/0x530
[ 119.253522][ T31] ? netdev_core_pick_tx+0x2a0/0x2a0
[ 119.253968][ T31] ? unwind_next_frame+0x3de/0x1c90
[ 119.254403][ T31] ? rcu_read_lock_sched_held+0xd/0xa0
[ 119.254872][ T31] ? lock_acquire+0x510/0x630
[ 119.255266][ T31] ? rcu_read_lock_sched_held+0xd/0xa0
[ 119.255724][ T31] ? lock_release+0x5a2/0x840
[ 119.256119][ T31] ? mroute6_is_socket+0x14d/0x220
[ 119.256549][ T31] ? rcu_read_lock_sched_held+0xd/0xa0
[ 119.257064][ T31] ? lock_acquire+0x510/0x630
[ 119.257507][ T31] ? rcu_read_lock_sched_held+0xd/0xa0
[ 119.258016][ T31] ? lock_release+0x5a2/0x840
[ 119.258410][ T31] ? lock_release+0x840/0x840
[ 119.258812][ T31] ? ip6_finish_output+0x779/0x1190
[ 119.259304][ T31] ? lock_downgrade+0x7b0/0x7b0
[ 119.259778][ T31] ? rcu_read_lock_bh_held+0xd/0x90
[ 119.260275][ T31] ? ___neigh_lookup_noref.constprop.0+0x266/0x6d0
[ 119.260896][ T31] ? ip6_finish_output2+0x861/0x1690
[ 119.261343][ T31] ip6_finish_output2+0x861/0x1690
[ 119.261776][ T31] ? lock_release+0x5a2/0x840
[ 119.262176][ T31] ? __kasan_kmalloc+0x7f/0xa0
[ 119.262574][ T31] ? ip6_mtu+0x139/0x320
[ 119.262942][ T31] ? ip6_frag_next+0xcc0/0xcc0
[ 119.263341][ T31] ? rcu_read_lock_sched_held+0xd/0xa0
[ 119.263816][ T31] ? nf_ct_netns_get+0xe0/0xe0
[ 119.264214][ T31] ? rcu_read_lock_sched_held+0xd/0xa0
[ 119.264679][ T31] ? lock_release+0x5a2/0x840
[ 119.265066][ T31] ? nf_conntrack_in.cold+0x9b/0xe2
[ 119.265504][ T31] ? ip6_output+0x4c9/0x6e0
[ 119.265891][ T31] ip6_finish_output+0x779/0x1190
[ 119.266315][ T31] ? nf_hook_slow+0xb1/0x170
[ 119.266711][ T31] ip6_output+0x1f4/0x6e0
[ 119.267069][ T31] ? rcu_read_lock_sched_held+0xd/0xa0
[ 119.267520][ T31] ? ip6_finish_output+0x1190/0x1190
[ 119.267969][ T31] ? NF_HOOK.constprop.0+0x1b3/0x5f0
[ 119.268415][ T31] ? lock_pin_lock+0x184/0x380
[ 119.268817][ T31] ? ip6_fragment+0x2910/0x2910
[ 119.269269][ T31] ? nf_ct_netns_do_get+0x6c0/0x6c0
[ 119.269773][ T31] ? nf_hook_slow+0xb1/0x170
[ 119.270211][ T31] NF_HOOK.constprop.0+0xe6/0x5f0
[ 119.270697][ T31] ? igmp6_mcf_seq_next+0x460/0x460
[ 119.271159][ T31] ? igmp6_mcf_seq_stop+0x130/0x130
[ 119.271600][ T31] ? icmp6_dst_alloc+0x3dc/0x610
[ 119.272030][ T31] mld_sendpack+0x619/0xb50
[ 119.272413][ T31] ? NF_HOOK.constprop.0+0x5f0/0x5f0
[ 119.272867][ T31] ? lock_release+0x840/0x840
[ 119.273254][ T31] mld_ifc_work+0x564/0xaa0
[ 119.273642][ T31] ? pwq_activate_inactive_work+0xb2/0x190
[ 119.274126][ T31] process_one_work+0x875/0x1440
[ 119.274543][ T31] ? lock_release+0x840/0x840
[ 119.274936][ T31] ? pwq_dec_nr_in_flight+0x230/0x230
[ 119.275381][ T31] ? rwlock_bug.part.0+0x90/0x90
[ 119.275801][ T31] worker_thread+0x598/0xec0
[ 119.276187][ T31] ? process_one_work+0x1440/0x1440
[ 119.276613][ T31] kthread+0x297/0x340
[ 119.276958][ T31] ? kthread_complete_and_exit+0x20/0x20
[ 119.277422][ T31] ret_from_fork+0x22/0x30
[ 119.277796][ T31] </TASK>
[ 119.278048][ T31] Modules linked in:
[ 119.278377][ T31] Dumping ftrace buffer:
[ 119.278733][ T31] (ftrace buffer empty)
[ 119.279151][ T31] ---[ end trace 0000000000000000 ]---
[ 119.279679][ T31] RIP: 0010:ebt_do_table+0x1dc/0x1ce0
[ 119.280187][ T31] Code: 89 fa 48 c1 ea 03 80 3c 02 00 0f 85 5c 16
00 00 48 b8 00 00 00 00 00 fc ff df 49 8b 6c df 08 48 8d 7d 2c 48 89 fa
48 c1 ea 03 <0f> b6 14 02 48 89 f8 83 e0 07 83 c0 03 38 d0 7c 08 84 d2
0f 85 88
[ 119.282006][ T31] RSP: 0018:ffffc90000347178 EFLAGS: 00010217
[ 119.282569][ T31] RAX: dffffc0000000000 RBX: 0000000000000003 RCX:
ffffffff8158677b
[ 119.283308][ T31] RDX: 0000000000000005 RSI: ffffffff889a7b80 RDI:
000000000000002c
[ 119.284056][ T31] RBP: 0000000000000000 R08: 0000000000000001 R09:
ffff888101a78443
[ 119.284811][ T31] R10: ffffed102034f088 R11: 000200100040dd86 R12:
ffffc90001111018
[ 119.285549][ T31] R13: ffffc90001103080 R14: ffffc90001111000 R15:
ffffc90001103000
[ 119.286284][ T31] FS: 0000000000000000(0000)
GS:ffff88811a380000(0000) knlGS:0000000000000000
[ 119.287119][ T31] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 119.287741][ T31] CR2: 00007f6a9adda000 CR3: 0000000100be3000 CR4:
00000000000006e0
[ 119.288489][ T31] Kernel panic - not syncing: Fatal exception in
interrupt
[ 119.298556][ T31] Dumping ftrace buffer:
[ 119.298974][ T31] (ftrace buffer empty)
[ 119.299399][ T31] Kernel Offset: disabled
[ 119.299823][ T31] Rebooting in 86400 seconds..
Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Reported-by: syzkaller <[email protected]>
Signed-off-by: Harshit Mogalapalli <[email protected]>
---
Testing is only done with reproducer.
net/bridge/netfilter/ebtables.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/net/bridge/netfilter/ebtables.c b/net/bridge/netfilter/ebtables.c
index f2dbefb61ce8..d19d439a66c5 100644
--- a/net/bridge/netfilter/ebtables.c
+++ b/net/bridge/netfilter/ebtables.c
@@ -217,6 +217,11 @@ unsigned int ebt_do_table(void *priv, struct sk_buff *skb,
else
cs = NULL;
chaininfo = private->hook_entry[hook];
+ if (!chaininfo) {
+ read_unlock_bh(&table->lock);
+ return NF_DROP;
+ }
+
nentries = private->hook_entry[hook]->nentries;
point = (struct ebt_entry *)(private->hook_entry[hook]->data);
counter_base = cb_base + private->hook_entry[hook]->counter_offset;
--
2.31.1
Harshit Mogalapalli <[email protected]> wrote:
> In ebt_do_table() function dereferencing 'private->hook_entry[hook]'
> can lead to NULL pointer dereference. So add a check to prevent that.
This looks incorrect, i.e. paperimg over the problem.
If hook_entry[hook] is NULL, how did this make it to the eval loop?
I guess ebtables lacks a sanity check on incoming ruleset?
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 <[email protected]>
Signed-off-by: Florian Westphal <[email protected]>
---
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);
--
2.37.2
On 20/08/22 9:56 pm, Florian Westphal wrote:
> Harshit Mogalapalli <[email protected]> wrote:
>> In ebt_do_table() function dereferencing 'private->hook_entry[hook]'
>> can lead to NULL pointer dereference. So add a check to prevent that.
>
Hi Florian,
Thanks a lot for checking the patch.
> This looks incorrect, i.e. paperimg over the problem.
>
Okay.
> If hook_entry[hook] is NULL, how did this make it to the eval loop?
>
When I run the reproducer and have 'private->hook_entry[hook]' printed,
it was NULL, so thought of adding a NULL check to prevent the NULL
dereference.
Attaching the C reproducer.
Regards,
Harshit
> I guess ebtables lacks a sanity check on incoming ruleset?
>
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 <[email protected]>
> Signed-off-by: Florian Westphal <[email protected]>
> ---
> 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/29/22 9:03 AM, Florian Westphal wrote:
> [email protected] <[email protected]> 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 <[email protected]>
>>> Signed-off-by: Florian Westphal <[email protected]>
>>
>> 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 !
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 <[email protected]>
> Signed-off-by: Florian Westphal <[email protected]>
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);
[email protected] <[email protected]> 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 <[email protected]>
> > Signed-off-by: Florian Westphal <[email protected]>
>
> 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