net/mlx4_en: Introduce flexible array to silence overflow warning

PATCH Applied Kees Cook 4 participants 5 messages Feb 18, 2023 – Feb 21, 2023

Subsystems (6)

Networking Network drivers InfiniBand Mellanox MLX4 core VPI driver +2 more subsystems

lore

Share & tools

lore

Kees Cook

author

2023-02-18 18:38 UTC #0 lore raw

PATCH 1/1 net/mlx4_en: Introduce flexible array to silence overflow warning

The call "skb_copy_from_linear_data(skb, inl + 1, spc)" triggers a FORTIFY memcpy() warning on ppc64 platform:

In function ‘fortify_memcpy_chk’,
inlined from ‘skb_copy_from_linear_data’ at ./include/linux/skbuff.h:4029:2, inlined from ‘build_inline_wqe’ at drivers/net/ethernet/mellanox/mlx4/en_tx.c:722:4, inlined from ‘mlx4_en_xmit’ at drivers/net/ethernet/mellanox/mlx4/en_tx.c:1066:3:
./include/linux/fortify-string.h:513:25: error: call to ‘__write_overflow_field’ declared with attribute warning: detected write beyond size of field (1st parameter); maybe use struct_group()? [-Werror=attribute-warning]

  513 |                         __write_overflow_field(p_size_field, size);
      |                         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Same behaviour on x86 you can get if you use "__always_inline" instead of "inline" for skb_copy_from_linear_data() in skbuff.h

The call here copies data into inlined tx destricptor, which has 104 bytes (MAX_INLINE) space for data payload. In this case "spc" is known in compile-time but the destination is used with hidden knowledge (real structure of destination is different from that the compiler can see). That cause the fortify warning because compiler can check bounds, but the real bounds are different. "spc" can't be bigger than 64 bytes (MLX4_INLINE_ALIGN), so the data can always fit into inlined tx descriptor. The fact that "inl" points into inlined tx descriptor is determined earlier in mlx4_en_xmit().

Avoid confusing the compiler with "inl + 1" constructions to get to past the inl header by introducing a flexible array "data" to the struct so that the compiler can see that we are not dealing with an array of inl structs, but rather, arbitrary data following the structure. There are no changes to the structure layout reported by pahole, and the resulting machine code is actually smaller.

Reported-by Josef Oskera <[email protected]>

Link https://lore.kernel.org/lkml/[email protected] open here

Fixes f68f2ff91512 ("fortify: Detect struct member overflows in memcpy() at compile-time")

Cc Tariq Toukan <[email protected]>

Cc "David S. Miller" <[email protected]>

Cc Eric Dumazet <[email protected]>

Cc Jakub Kicinski <[email protected]>

Cc Paolo Abeni <[email protected]>

Cc Yishai Hadas <[email protected]>

Cc [email protected]

Signed-off-by Kees Cook <[email protected]>

---
 drivers/net/ethernet/mellanox/mlx4/en_tx.c | 22 +++++++++++-----------
 include/linux/mlx4/qp.h                    |  1 +
 2 files changed, 12 insertions(+), 11 deletions(-)

drivers/net/ethernet/mellanox/mlx4/en_tx.c +11 ~~-11~~

index c5758637b7be..2f79378fbf6e 100644
@@ -699,32 +699,32 @@ static void build_inline_wqe(struct mlx4_en_tx_desc *tx_desc,
 			inl->byte_count = cpu_to_be32(1 << 31 | skb->len);
 		} else {
 			inl->byte_count = cpu_to_be32(1 << 31 | MIN_PKT_LEN);
-			memset(((void *)(inl + 1)) + skb->len, 0,
+			memset(inl->data + skb->len, 0,
 			       MIN_PKT_LEN - skb->len);
 		}
-		skb_copy_from_linear_data(skb, inl + 1, hlen);
+		skb_copy_from_linear_data(skb, inl->data, hlen);
 		if (shinfo->nr_frags)
-			memcpy(((void *)(inl + 1)) + hlen, fragptr,
+			memcpy(inl->data + hlen, fragptr,
 			       skb_frag_size(&shinfo->frags[0]));
 
 	} else {
 		inl->byte_count = cpu_to_be32(1 << 31 | spc);
 		if (hlen <= spc) {
-			skb_copy_from_linear_data(skb, inl + 1, hlen);
+			skb_copy_from_linear_data(skb, inl->data, hlen);
 			if (hlen < spc) {
-				memcpy(((void *)(inl + 1)) + hlen,
+				memcpy(inl->data + hlen,
 				       fragptr, spc - hlen);
 				fragptr +=  spc - hlen;
 			}
-			inl = (void *) (inl + 1) + spc;
-			memcpy(((void *)(inl + 1)), fragptr, skb->len - spc);
+			inl = (void *)inl->data + spc;
+			memcpy(inl->data, fragptr, skb->len - spc);
 		} else {
-			skb_copy_from_linear_data(skb, inl + 1, spc);
-			inl = (void *) (inl + 1) + spc;
-			skb_copy_from_linear_data_offset(skb, spc, inl + 1,
+			skb_copy_from_linear_data(skb, inl->data, spc);
+			inl = (void *)inl->data + spc;
+			skb_copy_from_linear_data_offset(skb, spc, inl->data,
 							 hlen - spc);
 			if (shinfo->nr_frags)
-				memcpy(((void *)(inl + 1)) + hlen - spc,
+				memcpy(inl->data + hlen - spc,
 				       fragptr,
 				       skb_frag_size(&shinfo->frags[0]));
 		}

include/linux/mlx4/qp.h +1 -0

index c78b90f2e9a1..b9a7b1319f5d 100644
@@ -446,6 +446,7 @@ enum {
 
 struct mlx4_wqe_inline_seg {
 	__be32			byte_count;
+	__u8			data[];
 };
 
 enum mlx4_update_qp_attr {

Signature · 1 line

2.34.1

Tariq Toukan

2023-02-19 09:45 UTC #1 lore raw

Quoted 45 lines · Kees Cook

The call "skb_copy_from_linear_data(skb, inl + 1, spc)" triggers a FORTIFY memcpy() warning on ppc64 platform:
In function ‘fortify_memcpy_chk’,
inlined from ‘skb_copy_from_linear_data’ at ./include/linux/skbuff.h:4029:2, inlined from ‘build_inline_wqe’ at drivers/net/ethernet/mellanox/mlx4/en_tx.c:722:4, inlined from ‘mlx4_en_xmit’ at drivers/net/ethernet/mellanox/mlx4/en_tx.c:1066:3:
./include/linux/fortify-string.h:513:25: error: call to ‘__write_overflow_field’ declared with attribute warning: detected write beyond size of field (1st parameter); maybe use struct_group()? [-Werror=attribute-warning]
   513 |                         __write_overflow_field(p_size_field, size);
       |                         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Same behaviour on x86 you can get if you use "__always_inline" instead of "inline" for skb_copy_from_linear_data() in skbuff.h
The call here copies data into inlined tx destricptor, which has 104 bytes (MAX_INLINE) space for data payload. In this case "spc" is known in compile-time but the destination is used with hidden knowledge (real structure of destination is different from that the compiler can see). That cause the fortify warning because compiler can check bounds, but the real bounds are different. "spc" can't be bigger than 64 bytes (MLX4_INLINE_ALIGN), so the data can always fit into inlined tx descriptor. The fact that "inl" points into inlined tx descriptor is determined earlier in mlx4_en_xmit().
Avoid confusing the compiler with "inl + 1" constructions to get to past the inl header by introducing a flexible array "data" to the struct so that the compiler can see that we are not dealing with an array of inl structs, but rather, arbitrary data following the structure. There are no changes to the structure layout reported by pahole, and the resulting machine code is actually smaller.
Reported-by Josef Oskera <[email protected]>
Link https://lore.kernel.org/lkml/[email protected] open here
Fixes f68f2ff91512 ("fortify: Detect struct member overflows in memcpy() at compile-time")
Cc Tariq Toukan <[email protected]>
Cc "David S. Miller" <[email protected]>
Cc Eric Dumazet <[email protected]>
Cc Jakub Kicinski <[email protected]>
Cc Paolo Abeni <[email protected]>
Cc Yishai Hadas <[email protected]>
Cc [email protected]
Cc [email protected]
Signed-off-by Kees Cook <[email protected]>

Quoted 5 lines

---
  drivers/net/ethernet/mellanox/mlx4/en_tx.c | 22 +++++++++++-----------
  include/linux/mlx4/qp.h                    |  1 +
  2 files changed, 12 insertions(+), 11 deletions(-)

Just saw your patch now, after commenting on the other thread. :)

So you choose not to fix similar usages in RDMA driver drivers/infiniband/hw/mlx4/qp.c, like:

3204 spc = MLX4_INLINE_ALIGN -
3205 ((unsigned long) (inl + 1) & (MLX4_INLINE_ALIGN - 1));
3206 if (header_size <= spc) {

3207                 inl->byte_count = cpu_to_be32(1 << 31 | header_size);
3208                 memcpy(inl + 1, sqp->header_buf, header_size);
3209                 i = 1;

3210 } else {
3211 inl->byte_count = cpu_to_be32(1 << 31 | spc);
3212 memcpy(inl + 1, sqp->header_buf, spc);
3213
3214 inl = (void *) (inl + 1) + spc;
3215 memcpy(inl + 1, sqp->header_buf + spc, header_size - spc);

This keeps the patch minimal indeed.

Did you repro the issue and test this solution?
Maybe Josef can also verify it works for him?

Quoted 30 lines

drivers/net/ethernet/mellanox/mlx4/en_tx.c +5 -5

index c5758637b7be..2f79378fbf6e 100644
@@ -699,32 +699,32 @@ static void build_inline_wqe(struct mlx4_en_tx_desc *tx_desc,
  			inl->byte_count = cpu_to_be32(1 << 31 | skb->len);
  		} else {
  			inl->byte_count = cpu_to_be32(1 << 31 | MIN_PKT_LEN);
-			memset(((void *)(inl + 1)) + skb->len, 0,
+			memset(inl->data + skb->len, 0,
  			       MIN_PKT_LEN - skb->len);
  		}
-		skb_copy_from_linear_data(skb, inl + 1, hlen);
+		skb_copy_from_linear_data(skb, inl->data, hlen);
  		if (shinfo->nr_frags)
-			memcpy(((void *)(inl + 1)) + hlen, fragptr,
+			memcpy(inl->data + hlen, fragptr,
  			       skb_frag_size(&shinfo->frags[0]));
 
  	} else {
  		inl->byte_count = cpu_to_be32(1 << 31 | spc);
  		if (hlen <= spc) {
-			skb_copy_from_linear_data(skb, inl + 1, hlen);
+			skb_copy_from_linear_data(skb, inl->data, hlen);
  			if (hlen < spc) {
-				memcpy(((void *)(inl + 1)) + hlen,
+				memcpy(inl->data + hlen,
  				       fragptr, spc - hlen);
  				fragptr +=  spc - hlen;
  			}

drivers/net/ethernet/mellanox/mlx4/en_tx.c static void build_inline_wqe(struct mlx4_en_tx_desc *tx_desc, · line 723

Quoted 10 lines

drivers/net/ethernet/mellanox/mlx4/en_tx.c +4 -5

-			inl = (void *) (inl + 1) + spc;
-			memcpy(((void *)(inl + 1)), fragptr, skb->len - spc);
+			inl = (void *)inl->data + spc;
+			memcpy(inl->data, fragptr, skb->len - spc);
  		} else {
-			skb_copy_from_linear_data(skb, inl + 1, spc);
-			inl = (void *) (inl + 1) + spc;
-			skb_copy_from_linear_data_offset(skb, spc, inl + 1,
+			skb_copy_from_linear_data(skb, inl->data, spc);
+			inl = (void *)inl->data + spc;

No need now for all these (void *) castings.

Quoted 20 lines

drivers/net/ethernet/mellanox/mlx4/en_tx.c +2 -1

+			skb_copy_from_linear_data_offset(skb, spc, inl->data,
  							 hlen - spc);
  			if (shinfo->nr_frags)
-				memcpy(((void *)(inl + 1)) + hlen - spc,
+				memcpy(inl->data + hlen - spc,
  				       fragptr,
  				       skb_frag_size(&shinfo->frags[0]));
  		}

include/linux/mlx4/qp.h +1 -0

index c78b90f2e9a1..b9a7b1319f5d 100644
@@ -446,6 +446,7 @@ enum {
 
  struct mlx4_wqe_inline_seg {
  	__be32			byte_count;
+	__u8			data[];
  };
 
  enum mlx4_update_qp_attr {

Josef Oskera

2023-02-20 02:10 UTC #2 lore raw

I've tried Kees's patch and it works for me. I am able to compile mlx4 without the fortify warning.

ne 19. 2. 2023 v 10:43 odesílatel Tariq Toukan <[email protected]> napsal:

Quoted 143 lines · Kees Cook

Quoted 45 lines
The call "skb_copy_from_linear_data(skb, inl + 1, spc)" triggers a FORTIFY memcpy() warning on ppc64 platform:
In function ‘fortify_memcpy_chk’,
inlined from ‘skb_copy_from_linear_data’ at ./include/linux/skbuff.h:4029:2, inlined from ‘build_inline_wqe’ at drivers/net/ethernet/mellanox/mlx4/en_tx.c:722:4, inlined from ‘mlx4_en_xmit’ at drivers/net/ethernet/mellanox/mlx4/en_tx.c:1066:3:
./include/linux/fortify-string.h:513:25: error: call to ‘__write_overflow_field’ declared with attribute warning: detected write beyond size of field (1st parameter); maybe use struct_group()? [-Werror=attribute-warning]
   513 |                         __write_overflow_field(p_size_field, size);
       |                         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Same behaviour on x86 you can get if you use "__always_inline" instead of "inline" for skb_copy_from_linear_data() in skbuff.h
The call here copies data into inlined tx destricptor, which has 104 bytes (MAX_INLINE) space for data payload. In this case "spc" is known in compile-time but the destination is used with hidden knowledge (real structure of destination is different from that the compiler can see). That cause the fortify warning because compiler can check bounds, but the real bounds are different. "spc" can't be bigger than 64 bytes (MLX4_INLINE_ALIGN), so the data can always fit into inlined tx descriptor. The fact that "inl" points into inlined tx descriptor is determined earlier in mlx4_en_xmit().
Avoid confusing the compiler with "inl + 1" constructions to get to past the inl header by introducing a flexible array "data" to the struct so that the compiler can see that we are not dealing with an array of inl structs, but rather, arbitrary data following the structure. There are no changes to the structure layout reported by pahole, and the resulting machine code is actually smaller.
Reported-by Josef Oskera <[email protected]>
Link https://lore.kernel.org/lkml/[email protected] open here
Fixes f68f2ff91512 ("fortify: Detect struct member overflows in memcpy() at compile-time")
Cc Tariq Toukan <[email protected]>
Cc "David S. Miller" <[email protected]>
Cc Eric Dumazet <[email protected]>
Cc Jakub Kicinski <[email protected]>
Cc Paolo Abeni <[email protected]>
Cc Yishai Hadas <[email protected]>
Cc [email protected]
Cc [email protected]
Signed-off-by Kees Cook <[email protected]>
Quoted 5 lines
---
  drivers/net/ethernet/mellanox/mlx4/en_tx.c | 22 +++++++++++-----------
  include/linux/mlx4/qp.h                    |  1 +
  2 files changed, 12 insertions(+), 11 deletions(-)
Just saw your patch now, after commenting on the other thread. :)
So you choose not to fix similar usages in RDMA driver drivers/infiniband/hw/mlx4/qp.c, like:
3204 spc = MLX4_INLINE_ALIGN -
3205 ((unsigned long) (inl + 1) & (MLX4_INLINE_ALIGN - 1));
3206 if (header_size <= spc) {
3207                 inl->byte_count = cpu_to_be32(1 << 31 | header_size);
3208                 memcpy(inl + 1, sqp->header_buf, header_size);
3209                 i = 1;
3210 } else {
3211 inl->byte_count = cpu_to_be32(1 << 31 | spc);
3212 memcpy(inl + 1, sqp->header_buf, spc);
3213
3214 inl = (void *) (inl + 1) + spc;
3215 memcpy(inl + 1, sqp->header_buf + spc, header_size
- spc);
This keeps the patch minimal indeed.
Did you repro the issue and test this solution?
Maybe Josef can also verify it works for him?
Quoted 30 lines
drivers/net/ethernet/mellanox/mlx4/en_tx.c +5 -5
index c5758637b7be..2f79378fbf6e 100644
@@ -699,32 +699,32 @@ static void build_inline_wqe(struct mlx4_en_tx_desc *tx_desc,
                      inl->byte_count = cpu_to_be32(1 << 31 | skb->len);
              } else {
                      inl->byte_count = cpu_to_be32(1 << 31 | MIN_PKT_LEN);
-                     memset(((void *)(inl + 1)) + skb->len, 0,
+                     memset(inl->data + skb->len, 0,
                             MIN_PKT_LEN - skb->len);
              }
-             skb_copy_from_linear_data(skb, inl + 1, hlen);
+             skb_copy_from_linear_data(skb, inl->data, hlen);
              if (shinfo->nr_frags)
-                     memcpy(((void *)(inl + 1)) + hlen, fragptr,
+                     memcpy(inl->data + hlen, fragptr,
                             skb_frag_size(&shinfo->frags[0]));
 
      } else {
              inl->byte_count = cpu_to_be32(1 << 31 | spc);
              if (hlen <= spc) {
-                     skb_copy_from_linear_data(skb, inl + 1, hlen);
+                     skb_copy_from_linear_data(skb, inl->data, hlen);
                      if (hlen < spc) {
-                             memcpy(((void *)(inl + 1)) + hlen,
+                             memcpy(inl->data + hlen,
                                     fragptr, spc - hlen);
                              fragptr +=  spc - hlen;
                      }
drivers/net/ethernet/mellanox/mlx4/en_tx.c static void build_inline_wqe(struct mlx4_en_tx_desc *tx_desc, · line 723
Quoted 10 lines
drivers/net/ethernet/mellanox/mlx4/en_tx.c +4 -5
-                     inl = (void *) (inl + 1) + spc;
-                     memcpy(((void *)(inl + 1)), fragptr, skb->len - spc);
+                     inl = (void *)inl->data + spc;
+                     memcpy(inl->data, fragptr, skb->len - spc);
              } else {
-                     skb_copy_from_linear_data(skb, inl + 1, spc);
-                     inl = (void *) (inl + 1) + spc;
-                     skb_copy_from_linear_data_offset(skb, spc, inl + 1,
+                     skb_copy_from_linear_data(skb, inl->data, spc);
+                     inl = (void *)inl->data + spc;
No need now for all these (void *) castings.
Quoted 20 lines
drivers/net/ethernet/mellanox/mlx4/en_tx.c +2 -1
+                     skb_copy_from_linear_data_offset(skb, spc, inl->data,
                                                       hlen - spc);
                      if (shinfo->nr_frags)
-                             memcpy(((void *)(inl + 1)) + hlen - spc,
+                             memcpy(inl->data + hlen - spc,
                                     fragptr,
                                     skb_frag_size(&shinfo->frags[0]));
              }
include/linux/mlx4/qp.h +1 -0
index c78b90f2e9a1..b9a7b1319f5d 100644
@@ -446,6 +446,7 @@ enum {
 
  struct mlx4_wqe_inline_seg {
      __be32                  byte_count;
+     __u8                    data[];
  };
 
  enum mlx4_update_qp_attr {

Tariq Toukan

Reviewed-by

2023-02-20 08:31 UTC #3 lore raw

Quoted 4 lines · Josef Oskera

I've tried Kees's patch and it works for me. I am able to compile mlx4 without the fortify warning.
ne 19. 2. 2023 v 10:43 odesílatel Tariq Toukan <[email protected]> napsal:

Thanks Josef.
Then let's go with this one.

Reviewed-by Tariq Toukan <[email protected]>

[email protected]

CI Applied

2023-02-21 00:40 UTC #4 lore raw

In reply to Kees Cook

This patch was applied to netdev/net-next.git (master) by Jakub Kicinski <[email protected]>:

Hello:

This patch was applied to netdev/net-next.git (master) by Jakub Kicinski <[email protected]>:

Quoted 10 lines · On Sat, 18 Feb 2023 10:38:50 -0800 you wrote:

The call "skb_copy_from_linear_data(skb, inl + 1, spc)" triggers a FORTIFY memcpy() warning on ppc64 platform:
In function ‘fortify_memcpy_chk’,
inlined from ‘skb_copy_from_linear_data’ at ./include/linux/skbuff.h:4029:2, inlined from ‘build_inline_wqe’ at drivers/net/ethernet/mellanox/mlx4/en_tx.c:722:4, inlined from ‘mlx4_en_xmit’ at drivers/net/ethernet/mellanox/mlx4/en_tx.c:1066:3:
./include/linux/fortify-string.h:513:25: error: call to ‘__write_overflow_field’ declared with attribute warning: detected write beyond size of field (1st parameter); maybe use struct_group()? [-Werror=attribute-warning]

Quoted 4 lines

  513 |                         __write_overflow_field(p_size_field, size);
      |                         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

[...]

Here is the summary with links:
- net/mlx4_en: Introduce flexible array to silence overflow warning

    https://git.kernel.org/netdev/net-next/c/f8f185e39b4d

You are awesome, thank you!

Signature · 2 lines

Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html