| Message ID | 20230927020221.85292-3-joao@overdrivepizza.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Prevent potential write out of bounds | expand |
From: joao@overdrivepizza.com > Sent: 27 September 2023 03:02 > > From: Joao Moreira <joao.moreira@intel.com> > > Currently, in nft_flow_rule_create function, num_actions is a signed > integer. Yet, it is processed within a loop which increments its > value. To prevent an overflow from occurring, make it unsigned and > also check if it reaches UINT_MAX when being incremented. > > After checking with maintainers, it was mentioned that front-end will > cap the num_actions value and that it is not possible to reach such > condition for an overflow. Yet, for correctness, it is still better to > fix this. > > This issue was observed by the commit author while reviewing a write-up > regarding a CVE within the same subsystem [1]. > > 1 - https://nickgregory.me/post/2022/03/12/cve-2022-25636/ > > Signed-off-by: Joao Moreira <joao.moreira@intel.com> > --- > net/netfilter/nf_tables_offload.c | 6 +++++- > 1 file changed, 5 insertions(+), 1 deletion(-) > > diff --git a/net/netfilter/nf_tables_offload.c b/net/netfilter/nf_tables_offload.c > index 12ab78fa5d84..d25088791a74 100644 > --- a/net/netfilter/nf_tables_offload.c > +++ b/net/netfilter/nf_tables_offload.c > @@ -90,7 +90,8 @@ struct nft_flow_rule *nft_flow_rule_create(struct net *net, > { > struct nft_offload_ctx *ctx; > struct nft_flow_rule *flow; > - int num_actions = 0, err; > + unsigned int num_actions = 0; > + int err; > struct nft_expr *expr; > > expr = nft_expr_first(rule); > @@ -99,6 +100,9 @@ struct nft_flow_rule *nft_flow_rule_create(struct net *net, > expr->ops->offload_action(expr)) > num_actions++; > > + if (num_actions == UINT_MAX) > + return ERR_PTR(-ENOMEM); > + > expr = nft_expr_next(expr); The code is going to 'crash and burn' well before the counter can possibly overflow. nft_expr_next() is ((void *)expr) + expr->ops->size; It is far more likely that has got setup wrong than the count is too big. David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
diff --git a/net/netfilter/nf_tables_offload.c b/net/netfilter/nf_tables_offload.c index 12ab78fa5d84..d25088791a74 100644 --- a/net/netfilter/nf_tables_offload.c +++ b/net/netfilter/nf_tables_offload.c @@ -90,7 +90,8 @@ struct nft_flow_rule *nft_flow_rule_create(struct net *net, { struct nft_offload_ctx *ctx; struct nft_flow_rule *flow; - int num_actions = 0, err; + unsigned int num_actions = 0; + int err; struct nft_expr *expr; expr = nft_expr_first(rule); @@ -99,6 +100,9 @@ struct nft_flow_rule *nft_flow_rule_create(struct net *net, expr->ops->offload_action(expr)) num_actions++; + if (num_actions == UINT_MAX) + return ERR_PTR(-ENOMEM); + expr = nft_expr_next(expr); }