Skip to contentSkip to conversation

net: move padding in struct skb_shared_info

PATCH NAK Alexey Dobriyan 3 participants 5 messages Apr 10, 2017 – Apr 11, 2017

Subsystems (2)
Networking Network drivers
lore
Share & tools
lore

Alexey Dobriyan

author
#0 lore raw

PATCH 1/1 net: move padding in struct skb_shared_info

commit 7f564528a480084e2318cd48caba7aef4a54a77f
("skbuff: Extend gso_type to unsigned int.") created padding as first field of struct skb_shared_info requiring [R64+imm8] addressing mode for all fields.

Patch bubbles up padding brinding code size down to original levels and even smaller:

add/remove: 0/0 grow/shrink: 4/304 up/down: 20/-720 (-700)
function old new delta
iwl_trans_pcie_tx 3994 4006 +12
tap_do_read 1070 1074 +4
packet_recvmsg 1155 1157 +2
be_xmit 2038 2040 +2
zerocopy_sg_from_iter 455 454 -1
...
__ef4_rx_packet 1358 1349 -9
hix5hd2_poll 1787 1777 -10
e1000_clean_jumbo_rx_irq 3599 3587 -12
skb_try_coalesce 1118 1105 -13
xenvif_tx_build_gops 5057 5043 -14

Signed-off-by Alexey Dobriyan <[email protected]>

---

 include/linux/skbuff.h |    1 -
 1 file changed, 1 deletion(-)
include/linux/skbuff.h +0 -1
@@ -413,7 +413,6 @@ struct ubuf_info {
  * the end of the header data, ie. at skb->end.
  */
 struct skb_shared_info {
-	unsigned short	_unused;
 	unsigned char	nr_frags;
 	__u8		tx_flags;
 	unsigned short	gso_size;
NAK
#1 lore raw
Quoted 31 lines · Alexey Dobriyan

commit 7f564528a480084e2318cd48caba7aef4a54a77f
("skbuff: Extend gso_type to unsigned int.") created padding as first field of struct skb_shared_info requiring [R64+imm8] addressing mode for all fields.

Patch bubbles up padding brinding code size down to original levels and even smaller:

add/remove: 0/0 grow/shrink: 4/304 up/down: 20/-720 (-700)
function old new delta
iwl_trans_pcie_tx 3994 4006 +12
tap_do_read 1070 1074 +4
packet_recvmsg 1155 1157 +2
be_xmit 2038 2040 +2
zerocopy_sg_from_iter 455 454 -1
...
__ef4_rx_packet 1358 1349 -9
hix5hd2_poll 1787 1777 -10
e1000_clean_jumbo_rx_irq 3599 3587 -12
skb_try_coalesce 1118 1105 -13
xenvif_tx_build_gops 5057 5043 -14

Signed-off-by Alexey Dobriyan <[email protected]>

---

 include/linux/skbuff.h |    1 -
 1 file changed, 1 deletion(-)

include/linux/skbuff.h struct ubuf_info { · line 418

Quoted 7 lines
include/linux/skbuff.h +0 -1
@@ -413,7 +413,6 @@ struct ubuf_info {
  * the end of the header data, ie. at skb->end.
  */
 struct skb_shared_info {
-	unsigned short	_unused;
 	unsigned char	nr_frags;
 	__u8		tx_flags;
 	unsigned short	gso_size;

Nack

This exact placement was discussed at Netconf and Netdev.

We had off-by-one errors in the past leading to nr_frags being mangled, and some exploits were quite happy to use these bugs.

Some shuffling in shared_info might help us to find other bugs, and give more work to security researchers

author
#2 lore raw
Quoted 9 lines · Eric Dumazet
Quoted 5 lines · Alexey Dobriyan

struct skb_shared_info {

-     unsigned short  _unused;
      unsigned char   nr_frags;
      __u8            tx_flags;
      unsigned short  gso_size;

Nack

Quoted 7 lines

This exact placement was discussed at Netconf and Netdev.

We had off-by-one errors in the past leading to nr_frags being mangled, and some exploits were quite happy to use these bugs.

Some shuffling in shared_info might help us to find other bugs, and give more work to security researchers

By this logic there should be redzone around every field in networking stack.

#3 lore raw
Quoted 12 lines · Alexey Dobriyan
Quoted 11 lines · Eric Dumazet
Quoted 5 lines · Alexey Dobriyan

struct skb_shared_info {

-     unsigned short  _unused;
      unsigned char   nr_frags;
      __u8            tx_flags;
      unsigned short  gso_size;

Nack

This exact placement was discussed at Netconf and Netdev.

Quoted 7 lines
Quoted 5 lines

We had off-by-one errors in the past leading to nr_frags being mangled, and some exploits were quite happy to use these bugs.

Some shuffling in shared_info might help us to find other bugs, and give more work to security researchers

By this logic there should be redzone around every field in networking stack.

Only shared_info immediately follows skb->head

For other structures, standard debug tools in SLUB/SLB/KASAN already take care of redzones.

maintainer
#4 lore raw
In reply to Alexey Dobriyan

From: Alexey Dobriyan <[email protected]>
Date: Tue, 11 Apr 2017 12:41:08 +0300

Quoted 12 lines · Eric Dumazet
Quoted 11 lines · Alexey Dobriyan
Quoted 5 lines

struct skb_shared_info {

-     unsigned short  _unused;
      unsigned char   nr_frags;
      __u8            tx_flags;
      unsigned short  gso_size;

Nack

This exact placement was discussed at Netconf and Netdev.

Quoted 7 lines
Quoted 5 lines

We had off-by-one errors in the past leading to nr_frags being mangled, and some exploits were quite happy to use these bugs.

Some shuffling in shared_info might help us to find other bugs, and give more work to security researchers

By this logic there should be redzone around every field in networking stack.

It is not a general rule, but a situation we specifically know about here for this structure and how exploits are actively being coded.

I would like to politely ask that we don't strive so strongly for "tree wide rules" that we fail to see the need for specific handling in specific situations as is the case here.

Thank you.