[net] netfilter: nf_tables: fix pointer math issue in nft_byteorder_eval()

  The problem is in nft_byteorder_eval() where we are iterating through a
loop and writing to dst[0], dst[1], dst[2] and so on...  On each
iteration we are writing 8 bytes.  But dst[] is an array of u32 so each
element only has space for 4 bytes.  That means that every iteration
overwrites part of the previous element.

I spotted this bug while reviewing commit caf3ef7468f7 ("netfilter:
nf_tables: prevent OOB access in nft_byteorder_eval") which is a related
issue.  I think that the reason we have not detected this bug in testing
is that most of time we only write one element.

Fixes: ce1e7989d989 ("netfilter: nft_byteorder: provide 64bit le/be conversion")
Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
---
 include/net/netfilter/nf_tables.h | 4 ++--
 net/netfilter/nft_byteorder.c     | 5 +++--
 net/netfilter/nft_meta.c          | 2 +-
 3 files changed, 6 insertions(+), 5 deletions(-)
  

Dan Carpenter <dan.carpenter@linaro.org> wrote:
> The problem is in nft_byteorder_eval() where we are iterating through a
> loop and writing to dst[0], dst[1], dst[2] and so on...  On each
> iteration we are writing 8 bytes.  But dst[] is an array of u32 so each
> element only has space for 4 bytes.  That means that every iteration
> overwrites part of the previous element.
> 
> I spotted this bug while reviewing commit caf3ef7468f7 ("netfilter:
> nf_tables: prevent OOB access in nft_byteorder_eval") which is a related
> issue.  I think that the reason we have not detected this bug in testing
> is that most of time we only write one element.

LGTM, thanks Dan.  We will route this via nf.git.
  

On Fri, Nov 03, 2023 at 10:18:01AM +0100, Florian Westphal wrote:
> Dan Carpenter <dan.carpenter@linaro.org> wrote:
> > The problem is in nft_byteorder_eval() where we are iterating through a
> > loop and writing to dst[0], dst[1], dst[2] and so on...  On each
> > iteration we are writing 8 bytes.  But dst[] is an array of u32 so each
> > element only has space for 4 bytes.  That means that every iteration
> > overwrites part of the previous element.
> > 
> > I spotted this bug while reviewing commit caf3ef7468f7 ("netfilter:
> > nf_tables: prevent OOB access in nft_byteorder_eval") which is a related
> > issue.  I think that the reason we have not detected this bug in testing
> > is that most of time we only write one element.
> 
> LGTM, thanks Dan.  We will route this via nf.git.

Thanks for your patch.

One question, is this update really required?

diff --git a/include/net/netfilter/nf_tables.h b/include/net/netfilter/nf_tables.h
index 3bbd13ab1ecf..b157c5cafd14 100644
--- a/include/net/netfilter/nf_tables.h
+++ b/include/net/netfilter/nf_tables.h
@@ -178,9 +178,9 @@ static inline __be32 nft_reg_load_be32(const u32 *sreg)
        return *(__force __be32 *)sreg;
 }

-static inline void nft_reg_store64(u32 *dreg, u64 val)
+static inline void nft_reg_store64(u64 *dreg, u64 val)
 {
-       put_unaligned(val, (u64 *)dreg);
+       put_unaligned(val, dreg);
 }

 static inline u64 nft_reg_load64(const u32 *sreg)

because one of the goals of nft_reg_store64() is to avoid that caller
casts the register to 64-bits.
  

Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> On Fri, Nov 03, 2023 at 10:18:01AM +0100, Florian Westphal wrote:
> > Dan Carpenter <dan.carpenter@linaro.org> wrote:
> > > The problem is in nft_byteorder_eval() where we are iterating through a
> > > loop and writing to dst[0], dst[1], dst[2] and so on...  On each
> > > iteration we are writing 8 bytes.  But dst[] is an array of u32 so each
> > > element only has space for 4 bytes.  That means that every iteration
> > > overwrites part of the previous element.
> > > 
> > > I spotted this bug while reviewing commit caf3ef7468f7 ("netfilter:
> > > nf_tables: prevent OOB access in nft_byteorder_eval") which is a related
> > > issue.  I think that the reason we have not detected this bug in testing
> > > is that most of time we only write one element.
> > 
> > LGTM, thanks Dan.  We will route this via nf.git.
> 
> Thanks for your patch.
> 
> One question, is this update really required?

I think so, yes.  Part of this bug here is that this helper-niceness
masks whats really happening in the caller (advancing in strides of
'u32', rather than 'u64').
  

On Fri, Nov 03, 2023 at 09:42:51AM +0300, Dan Carpenter wrote:
> The problem is in nft_byteorder_eval() where we are iterating through a
> loop and writing to dst[0], dst[1], dst[2] and so on...  On each
> iteration we are writing 8 bytes.  But dst[] is an array of u32 so each
> element only has space for 4 bytes.  That means that every iteration
> overwrites part of the previous element.
> 
> I spotted this bug while reviewing commit caf3ef7468f7 ("netfilter:
> nf_tables: prevent OOB access in nft_byteorder_eval") which is a related
> issue.  I think that the reason we have not detected this bug in testing
> is that most of time we only write one element.
> 
> Fixes: ce1e7989d989 ("netfilter: nft_byteorder: provide 64bit le/be conversion")
> Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
> ---
>  include/net/netfilter/nf_tables.h | 4 ++--
>  net/netfilter/nft_byteorder.c     | 5 +++--
>  net/netfilter/nft_meta.c          | 2 +-
>  3 files changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/include/net/netfilter/nf_tables.h b/include/net/netfilter/nf_tables.h
> index 3bbd13ab1ecf..b157c5cafd14 100644
> --- a/include/net/netfilter/nf_tables.h
> +++ b/include/net/netfilter/nf_tables.h
> @@ -178,9 +178,9 @@ static inline __be32 nft_reg_load_be32(const u32 *sreg)
>  	return *(__force __be32 *)sreg;
>  }
>  
> -static inline void nft_reg_store64(u32 *dreg, u64 val)
> +static inline void nft_reg_store64(u64 *dreg, u64 val)
>  {
> -	put_unaligned(val, (u64 *)dreg);
> +	put_unaligned(val, dreg);
>  }
>  
>  static inline u64 nft_reg_load64(const u32 *sreg)
> diff --git a/net/netfilter/nft_byteorder.c b/net/netfilter/nft_byteorder.c
> index e596d1a842f7..f6e791a68101 100644
> --- a/net/netfilter/nft_byteorder.c
> +++ b/net/netfilter/nft_byteorder.c
> @@ -38,13 +38,14 @@ void nft_byteorder_eval(const struct nft_expr *expr,
>  
>  	switch (priv->size) {
>  	case 8: {
> +		u64 *dst64 = (void *)dst;
>  		u64 src64;
>  
>  		switch (priv->op) {
>  		case NFT_BYTEORDER_NTOH:
>  			for (i = 0; i < priv->len / 8; i++) {
>  				src64 = nft_reg_load64(&src[i]);
> -				nft_reg_store64(&dst[i],
> +				nft_reg_store64(&dst64[i],
>  						be64_to_cpu((__force __be64)src64));
>  			}
>  			break;
> @@ -52,7 +53,7 @@ void nft_byteorder_eval(const struct nft_expr *expr,
>  			for (i = 0; i < priv->len / 8; i++) {
>  				src64 = (__force __u64)
>  					cpu_to_be64(nft_reg_load64(&src[i]));
> -				nft_reg_store64(&dst[i], src64);
> +				nft_reg_store64(&dst64[i], src64);
>  			}
>  			break;
>  		}

I stumbled upon this when the issue got a CVE id (sigh) and I share
Andrea's (Cc-ed) concern that the fix is incomplete. While the fix,
commit c301f0981fdd ("netfilter: nf_tables: fix pointer math issue in
nft_byteorder_eval()") now, fixes the destination side, src is still
a pointer to u32, i.e. we are reading 64-bit values with relative
offsets which are multiples of 32 bits.

Shouldn't we fix this as well, e.g. like indicated below?

Michal

------------------------------------------------------------------------------
diff --git a/include/net/netfilter/nf_tables.h b/include/net/netfilter/nf_tables.h
index 001226c34621..bb4b83ea2908 100644
--- a/include/net/netfilter/nf_tables.h
+++ b/include/net/netfilter/nf_tables.h
@@ -183,9 +183,9 @@ static inline void nft_reg_store64(u64 *dreg, u64 val)
 	put_unaligned(val, dreg);
 }
 
-static inline u64 nft_reg_load64(const u32 *sreg)
+static inline u64 nft_reg_load64(const u64 *sreg)
 {
-	return get_unaligned((u64 *)sreg);
+	return get_unaligned(sreg);
 }
 
 static inline void nft_data_copy(u32 *dst, const struct nft_data *src,
diff --git a/net/netfilter/nft_byteorder.c b/net/netfilter/nft_byteorder.c
index f6e791a68101..2a64c69ed507 100644
--- a/net/netfilter/nft_byteorder.c
+++ b/net/netfilter/nft_byteorder.c
@@ -39,21 +39,22 @@ void nft_byteorder_eval(const struct nft_expr *expr,
 	switch (priv->size) {
 	case 8: {
 		u64 *dst64 = (void *)dst;
-		u64 src64;
+		u64 *src64 = (void *)src;
+		u64 val64;
 
 		switch (priv->op) {
 		case NFT_BYTEORDER_NTOH:
 			for (i = 0; i < priv->len / 8; i++) {
-				src64 = nft_reg_load64(&src[i]);
+				val64 = nft_reg_load64(&src64[i]);
 				nft_reg_store64(&dst64[i],
-						be64_to_cpu((__force __be64)src64));
+						be64_to_cpu((__force __be64)val64));
 			}
 			break;
 		case NFT_BYTEORDER_HTON:
 			for (i = 0; i < priv->len / 8; i++) {
-				src64 = (__force __u64)
-					cpu_to_be64(nft_reg_load64(&src[i]));
-				nft_reg_store64(&dst64[i], src64);
+				val64 = (__force __u64)
+					cpu_to_be64(nft_reg_load64(&src64[i]));
+				nft_reg_store64(&dst64[i], val64);
 			}
 			break;
 		}
------------------------------------------------------------------------------
  

On Tue, Feb 06, 2024 at 11:43:36AM +0100, Michal Kubecek wrote:
> 
> I stumbled upon this when the issue got a CVE id (sigh) and I share
> Andrea's (Cc-ed) concern that the fix is incomplete. While the fix,
> commit c301f0981fdd ("netfilter: nf_tables: fix pointer math issue in
> nft_byteorder_eval()") now, fixes the destination side, src is still
> a pointer to u32, i.e. we are reading 64-bit values with relative
> offsets which are multiples of 32 bits.
> 
> Shouldn't we fix this as well, e.g. like indicated below?
> 

Yep.  You're right.  Could you send that as a patch.

regards,
dan carpenter
  

On Tue, Feb 06, 2024 at 02:04:10PM +0300, Dan Carpenter wrote:
> On Tue, Feb 06, 2024 at 11:43:36AM +0100, Michal Kubecek wrote:
> > 
> > I stumbled upon this when the issue got a CVE id (sigh) and I share
> > Andrea's (Cc-ed) concern that the fix is incomplete. While the fix,
> > commit c301f0981fdd ("netfilter: nf_tables: fix pointer math issue in
> > nft_byteorder_eval()") now, fixes the destination side, src is still
> > a pointer to u32, i.e. we are reading 64-bit values with relative
> > offsets which are multiples of 32 bits.
> > 
> > Shouldn't we fix this as well, e.g. like indicated below?
> > 
> 
> Yep.  You're right.  Could you send that as a patch.

Thank you for checking. I'll send a patch in a moment.

Michal
  

Michal Kubecek <mkubecek@suse.cz> wrote:
> I stumbled upon this when the issue got a CVE id (sigh) and I share
> Andrea's (Cc-ed) concern that the fix is incomplete. While the fix,
> commit c301f0981fdd ("netfilter: nf_tables: fix pointer math issue in
> nft_byteorder_eval()") now, fixes the destination side, src is still
> a pointer to u32, i.e. we are reading 64-bit values with relative
> offsets which are multiples of 32 bits.
> 
> Shouldn't we fix this as well, e.g. like indicated below?

No, please remove multi-elem support instead, nothing uses this feature.
  

On Tue, Feb 06, 2024 at 12:11:12PM +0100, Florian Westphal wrote:
> Michal Kubecek <mkubecek@suse.cz> wrote:
> > I stumbled upon this when the issue got a CVE id (sigh) and I share
> > Andrea's (Cc-ed) concern that the fix is incomplete. While the fix,
> > commit c301f0981fdd ("netfilter: nf_tables: fix pointer math issue in
> > nft_byteorder_eval()") now, fixes the destination side, src is still
> > a pointer to u32, i.e. we are reading 64-bit values with relative
> > offsets which are multiples of 32 bits.
> > 
> > Shouldn't we fix this as well, e.g. like indicated below?
> 
> No, please remove multi-elem support instead, nothing uses this feature.

See attached patch.

I posted this:

https://patchwork.ozlabs.org/project/netfilter-devel/patch/20240202120602.5122-1-pablo@netfilter.org/

I can replace it by the attached patch if you prefer. This can only
help in the future to microoptimize some set configurations, where
several byteorder can be coalesced into one single expression.
  

On Tue, Feb 06, 2024 at 12:29:51PM +0100, Pablo Neira Ayuso wrote:
> On Tue, Feb 06, 2024 at 12:11:12PM +0100, Florian Westphal wrote:
> > Michal Kubecek <mkubecek@suse.cz> wrote:
> > > I stumbled upon this when the issue got a CVE id (sigh) and I share
> > > Andrea's (Cc-ed) concern that the fix is incomplete. While the fix,
> > > commit c301f0981fdd ("netfilter: nf_tables: fix pointer math issue in
> > > nft_byteorder_eval()") now, fixes the destination side, src is still
> > > a pointer to u32, i.e. we are reading 64-bit values with relative
> > > offsets which are multiples of 32 bits.
> > > 
> > > Shouldn't we fix this as well, e.g. like indicated below?
> > 
> > No, please remove multi-elem support instead, nothing uses this feature.
> 
> See attached patch.
> 
> I posted this:
> 
> https://patchwork.ozlabs.org/project/netfilter-devel/patch/20240202120602.5122-1-pablo@netfilter.org/
> 
> I can replace it by the attached patch if you prefer. This can only
> help in the future to microoptimize some set configurations, where
> several byteorder can be coalesced into one single expression.

I have to replace those index 'i' by simply dst instead, this is
obviosly incomplete.

> diff --git a/net/netfilter/nft_byteorder.c b/net/netfilter/nft_byteorder.c
> index 8cf91e47fd7a..af3412602869 100644
> --- a/net/netfilter/nft_byteorder.c
> +++ b/net/netfilter/nft_byteorder.c
> @@ -43,18 +43,14 @@ void nft_byteorder_eval(const struct nft_expr *expr,
>  
>  		switch (priv->op) {
>  		case NFT_BYTEORDER_NTOH:
> -			for (i = 0; i < priv->len / 8; i++) {
> -				src64 = nft_reg_load64(&src[i]);
> -				nft_reg_store64(&dst64[i],
> -						be64_to_cpu((__force __be64)src64));
> -			}
> +			src64 = nft_reg_load64(&src[i]);
> +			nft_reg_store64(&dst64[i],
> +					be64_to_cpu((__force __be64)src64));
>  			break;
>  		case NFT_BYTEORDER_HTON:
> -			for (i = 0; i < priv->len / 8; i++) {
> -				src64 = (__force __u64)
> -					cpu_to_be64(nft_reg_load64(&src[i]));
> -				nft_reg_store64(&dst64[i], src64);
> -			}
> +			src64 = (__force __u64)
> +				cpu_to_be64(nft_reg_load64(&src[i]));
> +			nft_reg_store64(&dst64[i], src64);
>  			break;
>  		}
>  		break;
> @@ -62,24 +58,20 @@ void nft_byteorder_eval(const struct nft_expr *expr,
>  	case 4:
>  		switch (priv->op) {
>  		case NFT_BYTEORDER_NTOH:
> -			for (i = 0; i < priv->len / 4; i++)
> -				dst[i] = ntohl((__force __be32)src[i]);
> +			dst[i] = ntohl((__force __be32)src[i]);
>  			break;
>  		case NFT_BYTEORDER_HTON:
> -			for (i = 0; i < priv->len / 4; i++)
> -				dst[i] = (__force __u32)htonl(src[i]);
> +			dst[i] = (__force __u32)htonl(src[i]);
>  			break;
>  		}
>  		break;
>  	case 2:
>  		switch (priv->op) {
>  		case NFT_BYTEORDER_NTOH:
> -			for (i = 0; i < priv->len / 2; i++)
> -				d16[i] = ntohs((__force __be16)s16[i]);
> +			d16[i] = ntohs((__force __be16)s16[i]);
>  			break;
>  		case NFT_BYTEORDER_HTON:
> -			for (i = 0; i < priv->len / 2; i++)
> -				d16[i] = (__force __u16)htons(s16[i]);
> +			d16[i] = (__force __u16)htons(s16[i]);
>  			break;
>  		}
>  		break;
> @@ -137,6 +129,9 @@ static int nft_byteorder_init(const struct nft_ctx *ctx,
>  	if (err < 0)
>  		return err;
>  
> +	if (priv->size != len)
> +		return -EINVAL;
> +
>  	priv->len = len;
>  
>  	if (len % size != 0)