[PATCH nf] netfilter: nf_tables: fix oob access

[PATCH nf] netfilter: nf_tables: fix oob access

[Florian Westphal]

BUG: KASAN: slab-out-of-bounds in nf_tables_rule_destroy+0xf1/0x130 at addr ffff88006a4c35c8
Read of size 8 by task nft/1607

When we've destroyed last valid expr, nft_expr_next() returns an invalid expr.
We must not dereference it unless it passes != nft_expr_last() check.

Signed-off-by: Florian Westphal <fw@strlen.de>
---
 I dislike nft_expr_last() naming, it doesn't return last
 valid expression but an invalid address...

 net/netfilter/nf_tables_api.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
index a019a87e58ee..0db5f9782265 100644
--- a/net/netfilter/nf_tables_api.c
+++ b/net/netfilter/nf_tables_api.c
@@ -2115,7 +2115,7 @@ static void nf_tables_rule_destroy(const struct nft_ctx *ctx,
 	 * is called on error from nf_tables_newrule().
 	 */
 	expr = nft_expr_first(rule);
-	while (expr->ops && expr != nft_expr_last(rule)) {
+	while (expr != nft_expr_last(rule) && expr->ops) {
 		nf_tables_expr_destroy(ctx, expr);
 		expr = nft_expr_next(expr);
 	}
-- 
2.7.3

Re: [PATCH nf] netfilter: nf_tables: fix oob access

[Pablo Neira Ayuso]

On Tue, Dec 13, 2016 at 01:59:33PM +0100, Florian Westphal wrote:
> BUG: KASAN: slab-out-of-bounds in nf_tables_rule_destroy+0xf1/0x130 at addr ffff88006a4c35c8
> Read of size 8 by task nft/1607
> 
> When we've destroyed last valid expr, nft_expr_next() returns an invalid expr.
> We must not dereference it unless it passes != nft_expr_last() check.
> 
> Signed-off-by: Florian Westphal <fw@strlen.de>
> ---
>  I dislike nft_expr_last() naming, it doesn't return last
>  valid expression but an invalid address...

Sure, send a patch for this, or simply update this oneliner in v2,
your call.  Thanks.

>  net/netfilter/nf_tables_api.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
> index a019a87e58ee..0db5f9782265 100644
> --- a/net/netfilter/nf_tables_api.c
> +++ b/net/netfilter/nf_tables_api.c
> @@ -2115,7 +2115,7 @@ static void nf_tables_rule_destroy(const struct nft_ctx *ctx,
>  	 * is called on error from nf_tables_newrule().
>  	 */
>  	expr = nft_expr_first(rule);
> -	while (expr->ops && expr != nft_expr_last(rule)) {
> +	while (expr != nft_expr_last(rule) && expr->ops) {
>  		nf_tables_expr_destroy(ctx, expr);
>  		expr = nft_expr_next(expr);
>  	}
> -- 
> 2.7.3
> 
> --
> To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH nf] netfilter: nf_tables: fix oob access

[Florian Westphal]

Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> On Tue, Dec 13, 2016 at 01:59:33PM +0100, Florian Westphal wrote:
> > BUG: KASAN: slab-out-of-bounds in nf_tables_rule_destroy+0xf1/0x130 at addr ffff88006a4c35c8
> > Read of size 8 by task nft/1607
> > 
> > When we've destroyed last valid expr, nft_expr_next() returns an invalid expr.
> > We must not dereference it unless it passes != nft_expr_last() check.
> > 
> > Signed-off-by: Florian Westphal <fw@strlen.de>
> > ---
> >  I dislike nft_expr_last() naming, it doesn't return last
> >  valid expression but an invalid address...
> 
> Sure, send a patch for this, or simply update this oneliner in v2,
> your call.  Thanks.

Ok, I will try to come up with a better name and send a patch once
next opens again. (what about rename to nft_expr_end()?)