netdev - Re: [PATCH] netfilter: nf_tables: prevent OOB access in nft_byteorder

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20230705201232.GG3751@breakpoint.cc>
Date: Wed, 5 Jul 2023 22:12:32 +0200
From: Florian Westphal <fw@...len.de>
To: Thadeu Lima de Souza Cascardo <cascardo@...onical.com>
Cc: Florian Westphal <fw@...len.de>, netfilter-devel@...r.kernel.org,
	netdev@...r.kernel.org, Pablo Neira Ayuso <pablo@...filter.org>
Subject: Re: [PATCH] netfilter: nf_tables: prevent OOB access in
 nft_byteorder_eval

Thadeu Lima de Souza Cascardo <cascardo@...onical.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.