[PATCH] netfilter: nft_set_pipapo: fix stack out-of-bounds read in pipapo_drop()

[PATCH] netfilter: nft_set_pipapo: fix stack out-of-bounds read in pipapo_drop()

[Jenny Guanni Qu]

pipapo_drop() passes rulemap[i + 1].n to pipapo_unmap() as the
to_offset argument on every iteration, including the last one where
i == m->field_count - 1. This reads one element past the end of the
stack-allocated rulemap array (declared as rulemap[NFT_PIPAPO_MAX_FIELDS]
with NFT_PIPAPO_MAX_FIELDS == 16).

Although pipapo_unmap() returns early when is_last is true without
using the to_offset value, the argument is evaluated at the call site
before the function body executes, making this a genuine out-of-bounds
stack read confirmed by KASAN:

  BUG: KASAN: stack-out-of-bounds in pipapo_drop+0x50c/0x57c [nf_tables]
  Read of size 4 at addr ffff8000810e71a4

  This frame has 1 object:
   [32, 160) 'rulemap'

  The buggy address is at offset 164 -- exactly 4 bytes past the end
  of the rulemap array.

Pass 0 instead of rulemap[i + 1].n on the last iteration to avoid
the out-of-bounds read.

Fixes: 3c4287f62044 ("nf_tables: Add set type for arbitrary concatenation of ranges")
Signed-off-by: Jenny Guanni Qu <qguanni@gmail.com>
---
 net/netfilter/nft_set_pipapo.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/net/netfilter/nft_set_pipapo.c b/net/netfilter/nft_set_pipapo.c
index 7ef4b44471d3..9fb83fc05848 100644
--- a/net/netfilter/nft_set_pipapo.c
+++ b/net/netfilter/nft_set_pipapo.c
@@ -1659,7 +1659,8 @@ static void pipapo_drop(struct nft_pipapo_match *m,
 		}
 
 		pipapo_unmap(f->mt, f->rules, rulemap[i].to, rulemap[i].n,
-			     rulemap[i + 1].n, i == m->field_count - 1);
+			     i == m->field_count - 1 ? 0 : rulemap[i + 1].n,
+			     i == m->field_count - 1);
 		if (pipapo_resize(f, f->rules, f->rules - rulemap[i].n)) {
 			/* We can ignore this, a failure to shrink tables down
 			 * doesn't make tables invalid.
-- 
2.34.1

Re: [PATCH] netfilter: nft_set_pipapo: fix stack out-of-bounds read in pipapo_drop()

[Florian Westphal]

Jenny Guanni Qu <qguanni@gmail.com> wrote:
> pipapo_drop() passes rulemap[i + 1].n to pipapo_unmap() as the
> to_offset argument on every iteration, including the last one where
> i == m->field_count - 1. This reads one element past the end of the
> stack-allocated rulemap array (declared as rulemap[NFT_PIPAPO_MAX_FIELDS]
> with NFT_PIPAPO_MAX_FIELDS == 16).

Thanks, patch looks correct to me.

> diff --git a/net/netfilter/nft_set_pipapo.c b/net/netfilter/nft_set_pipapo.c
> index 7ef4b44471d3..9fb83fc05848 100644
> --- a/net/netfilter/nft_set_pipapo.c
> +++ b/net/netfilter/nft_set_pipapo.c
> @@ -1659,7 +1659,8 @@ static void pipapo_drop(struct nft_pipapo_match *m,
>  		}
>  
>  		pipapo_unmap(f->mt, f->rules, rulemap[i].to, rulemap[i].n,
> -			     rulemap[i + 1].n, i == m->field_count - 1);
> +			     i == m->field_count - 1 ? 0 : rulemap[i + 1].n,
> +			     i == m->field_count - 1);

Small nit, could you add

	bool last = i == m->field_count - 1;

and then use 'last ? 0 : ..., last) ?

This idiom is used elsewhere in the file as well and I think it makes
this sligthly more readable.

Thanks!