netdev - Re: [PATCH v2] 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]

Date: Thu, 2 Nov 2023 11:28:46 +0100
From: Florian Westphal <fw@...len.de>
To: Dan Carpenter <dan.carpenter@...aro.org>
Cc: Thadeu Lima de Souza Cascardo <cascardo@...onical.com>,
	netfilter-devel@...r.kernel.org, netdev@...r.kernel.org,
	Pablo Neira Ayuso <pablo@...filter.org>,
	Florian Westphal <fw@...len.de>,
	Harshit Mogalapalli <harshit.m.mogalapalli@...il.com>
Subject: Re: [PATCH v2] netfilter: nf_tables: prevent OOB access in
 nft_byteorder_eval

Dan Carpenter <dan.carpenter@...aro.org> wrote:
> This patch is correct, but shouldn't we fix the code for 64 bit writes
> as well?

Care to send a patch?

> net/netfilter/nft_byteorder.c
>     26  void nft_byteorder_eval(const struct nft_expr *expr,
>     27                          struct nft_regs *regs,
>     28                          const struct nft_pktinfo *pkt)
>     29  {
>     30          const struct nft_byteorder *priv = nft_expr_priv(expr);
>     31          u32 *src = &regs->data[priv->sreg];
>     32          u32 *dst = &regs->data[priv->dreg];
>     33          u16 *s16, *d16;
>     34          unsigned int i;
>     35  
>     36          s16 = (void *)src;
>     37          d16 = (void *)dst;
>     38  
>     39          switch (priv->size) {
>     40          case 8: {
>     41                  u64 src64;
>     42  
>     43                  switch (priv->op) {
>     44                  case NFT_BYTEORDER_NTOH:
>     45                          for (i = 0; i < priv->len / 8; i++) {
>     46                                  src64 = nft_reg_load64(&src[i]);
>     47                                  nft_reg_store64(&dst[i],
>     48                                                  be64_to_cpu((__force __be64)src64));
> 
> We're writing 8 bytes, then moving forward 4 bytes and writing 8 bytes
> again.  Each subsequent write over-writes 4 bytes from the previous
> write.

Yes.  I can't think if a case where we'd do two swaps back-to-back,
which is probably the reason noone noticed this so far.