netfilter: nf_tables: prevent shift wrap in nft_chain_parse_hook()
Subsystems (3)
PATCH 1/1 netfilter: nf_tables: prevent shift wrap in nft_chain_parse_hook()
I believe that "hook->num" can be up to UINT_MAX so the shift has to be unsigned or shifting more than 31 is undefined. It's possible that this leads to array overflow in nf_tables_addchain():
ops->hook = hook.type->hooks[ops->hooknum];
--- net/netfilter/nf_tables_api.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
net/netfilter/nf_tables_api.c +1 -1
@@ -1545,7 +1545,7 @@ static int nft_chain_parse_hook(struct net *net,if (IS_ERR (type))return PTR_ERR (type); } -if (!(type->hook_mask & (1 << hook->num))) +if (!(type->hook_mask & (1U << hook->num)))return -EOPNOTSUPP ; if (type->type = NFT_CHAIN_T_NAT &&
Signature · 1 line
2.17.1
Quoted 17 lines · Dan Carpenter
I believe that "
hook->num" can be up toUINT_MAXso the shift has to be unsigned or shifting more than 31 is undefined. It's possible that this leads to array overflow innf_tables_addchain():ops->hook = hook.type->hooks[ops->hooknum];Fixes fe19c04ca137 ("netfilter: nf_tables: remove nhooks field from struct nft_af_info")Signed-off-by Dan Carpenter <[email protected]>--- net/netfilter/nf_tables_api.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)net/netfilter/nf_tables_api.c +0
-0
net/netfilter/nf_tables_api.c static int nft_chain_parse_hook(struct net *net, · line 1548
Quoted 5 lines
net/netfilter/nf_tables_api.c +1
-1@@ -1545,7 +1545,7 @@ static int nft_chain_parse_hook(struct net *net,if (IS_ERR (type))return PTR_ERR (type); } -if (!(type->hook_mask & (1 << hook->num))) +if (!(type->hook_mask & (1U << hook->num)))
Ugh... No this doesn't work. It's still undefined behavoir even if we make it unsigned. It has to be:
if (hook->num > 31 || !(type->hook_mask & (1 << hook->num)))
which seems like a slightly ugly magic number but it fixes the buffer overflow.
regards,
dan carpenter
Florian Westphal
maintainerQuoted 4 lines · Dan Carpenter
if (hook->num > 31 || !(type->hook_mask & (1 << hook->num)))which seems like a slightly ugly magic number but it fixes the buffer overflow.
You can use NF_MAX_HOOKS instead of 31.
PATCH v2 1/1 netfilter: nf_tables: prevent shift wrap in nft_chain_parse_hook()
In reply to Dan CarpenterI believe that "hook->num" can be up to UINT_MAX. Shifting more than 31 bits would is undefined in C but in practice it would lead to shift wrapping. That would lead to an array overflow in nf_tables_addchain():
ops->hook = hook.type->hooks[ops->hooknum];
---
v2: In my original fix I thought making shifts unsigned was defined in C but it's not. My original patch didn't fix or change runtime at all.
Thanks to Florian Westphal for his help on this.
net/netfilter/nf_tables_api.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
net/netfilter/nf_tables_api.c +1 -1
@@ -1545,7 +1545,7 @@ static int nft_chain_parse_hook(struct net *net,if (IS_ERR (type))return PTR_ERR (type); } -if (!(type->hook_mask & (1 << hook->num))) +if (hook->num >NF_MAX_HOOKS || !(type->hook_mask & (1 << hook->num)))return -EOPNOTSUPP ; if (type->type = NFT_CHAIN_T_NAT &&
Signature · 1 line
2.17.1
Quoted 5 lines · Dan Carpenter
I believe that "
hook->num" can be up toUINT_MAX. Shifting more than 31 bits would is undefined in C but in practice it would lead to shift wrapping. That would lead to an array overflow innf_tables_addchain():ops->hook = hook.type->hooks[ops->hooknum];
Applied, thanks.