Re: [PATCH] netfilter: nf_tables: prevent OOB access in nft_byteorder_eval

[Florian Westphal <fw@strlen.de>]

Thadeu Lima de Souza Cascardo <cascardo@canonical.com> wrote:
> > > @@ -74,11 +77,11 @@ void nft_byteorder_eval(const struct nft_expr *expr,
> > >  		switch (priv->op) {
> > >  		case NFT_BYTEORDER_NTOH:
> > >  			for (i = 0; i < priv->len / 2; i++)
> > > -				d[i].u16 = ntohs((__force __be16)s[i].u16);
> > > +				d16[i] = ntohs((__force __be16)s16[i]);
> > 
> > This on the other hand... I'd say this should mimic what the 64bit
> > case is doing and use nft_reg_store16() nft_reg_load16() helpers for
> > the register accesses.
> > 
> > something like:
> > 
> > for (i = 0; i < priv->len / 2; i++) {
> >      v16 = nft_reg_load16(&src[i]);
> >      nft_reg_store16(&dst[i], + ntohs((__force __be16)v16));
> > }
> > 
> 
> The problem here is that we cannot index the 32-bit dst and src pointers as if
> they were 16-bit pointers. We will end up with the exact same problem we are
> trying to fix here.
> 
> I can change the code to use the accessors, but they use u32 pointers, so it
> would end up looking like:
> 
>  		case NFT_BYTEORDER_NTOH:
>  			for (i = 0; i < priv->len / 4; i++)
> -				d[i].u32 = ntohl((__force __be32)s[i].u32);
> +				dst[i] = ntohl((__force __be32)src[i]);
>  			break;
>  		case NFT_BYTEORDER_HTON:
>  			for (i = 0; i < priv->len / 4; i++)
> -				d[i].u32 = (__force __u32)htonl(s[i].u32);
> +				dst[i] = (__force __u32)htonl(src[i]);

Ack, thanks.

>  		case NFT_BYTEORDER_NTOH:
> -			for (i = 0; i < priv->len / 2; i++)
> -				d[i].u16 = ntohs((__force __be16)s[i].u16);
> +			for (i = 0; i < priv->len / 2; i++) {
> +				__be16 src16;
> +				src16 = nft_reg_load_be16((u32 *)&s16[i]);
> +				nft_reg_store_be16((u32 *)&d16[i], ntohs(src16));
> +			}

These accessors take a registers' address, not something in-between.

I think your original was better after all and we need to rely on whatever
expression filled the register to have done the right thing.