Re: [PATCH v4 net-next 12/12] mlx5: support BIG TCP packets

[Kees Cook <keescook@chromium.org>]

On Fri, May 06, 2022 at 07:43:13PM -0700, Eric Dumazet wrote:
> On Fri, May 6, 2022 at 7:37 PM Jakub Kicinski <kuba@kernel.org> wrote:
> >
> > On Fri, 6 May 2022 19:10:48 -0700 Eric Dumazet wrote:
> > > On Fri, May 6, 2022 at 6:54 PM Jakub Kicinski <kuba@kernel.org> wrote:
> > > > Without our patches drivers/net/ethernet/mellanox/mlx5/core/ builds
> > > > cleanly. Gotta be the new W=1 filed overflow warnings, let's bother
> > > > Kees.
> > >
> > > Note that inline_hdr.start is a 2 byte array.
> > >
> > > Obviously mlx5 driver copies more than 2 bytes of inlined headers.
> > >
> > > mlx5e_insert_vlan(eseg->inline_hdr.start, skb, attr->ihs)
> > > is called already with attr->ihs > 2
> > >
> > > So it should already complain ?
> >
> > It's a static checker, I presume it ignores attr->ihs because
> > it can't prove its value is indeed > 2. Unpleasant :/
> 
> Well, the unpleasant thing is that I do not see a way to get rid of
> this warning.
> Networking is full of variable sized headers.

So... this _is_ supposed to be copying off the end of struct vlan_ethhdr?
In that case, either don't use the vhdr cast, or add a flex array to
the end of the header. e.g. (untested):

diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_tx.c b/drivers/net/ethernet/mellanox/mlx5/core/en_tx.c
index 2dc48406cd08..990476b2e595 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_tx.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_tx.c
@@ -94,13 +94,18 @@ static inline u16 mlx5e_calc_min_inline(enum mlx5_inline_modes mode,
 static inline void mlx5e_insert_vlan(void *start, struct sk_buff *skb, u16 ihs)
 {
 	struct vlan_ethhdr *vhdr = (struct vlan_ethhdr *)start;
-	int cpy1_sz = 2 * ETH_ALEN;
-	int cpy2_sz = ihs - cpy1_sz;
+	void *data = skb->data;
+	const u16 cpy1_sz = sizeof(vhdr->addrs);
+	const u16 cpy2_sz = sizeof(vhdr->h_vlan_encapsulated_proto);
+	const u16 cpy3_sz = ihs - cpy1_sz - cpy2_sz;
 
-	memcpy(&vhdr->addrs, skb->data, cpy1_sz);
+	memcpy(&vhdr->addrs, data, cpy1_sz);
+	data += sizeof(cpy1_sz);
 	vhdr->h_vlan_proto = skb->vlan_proto;
 	vhdr->h_vlan_TCI = cpu_to_be16(skb_vlan_tag_get(skb));
-	memcpy(&vhdr->h_vlan_encapsulated_proto, skb->data + cpy1_sz, cpy2_sz);
+	memcpy(&vhdr->h_vlan_encapsulated_proto, data, cpy2_sz);
+	data += sizeof(cpy2_sz);
+	memcpy(&vhdr->h_vlan_contents, data, cpy3_sz);
 }
 
 static inline void
diff --git a/include/linux/if_vlan.h b/include/linux/if_vlan.h
index 2be4dd7e90a9..8178e20ce5b3 100644
--- a/include/linux/if_vlan.h
+++ b/include/linux/if_vlan.h
@@ -44,6 +44,7 @@ struct vlan_hdr {
  *	@h_vlan_proto: ethernet protocol
  *	@h_vlan_TCI: priority and VLAN ID
  *	@h_vlan_encapsulated_proto: packet type ID or len
+ *	@h_vlan_contents: The rest of the packet
  */
 struct vlan_ethhdr {
 	struct_group(addrs,
@@ -53,6 +54,7 @@ struct vlan_ethhdr {
 	__be16		h_vlan_proto;
 	__be16		h_vlan_TCI;
 	__be16		h_vlan_encapsulated_proto;
+	u8		h_vlan_contents[];
 };
 
 #include <linux/skbuff.h>


I'm still learning the skb helpers, but shouldn't this be using something
similar to skb_pull() that would do bounds checking, etc? Open-coded
accesses of skb->data have shown a repeated pattern of being a source
of flaws:
https://github.com/KSPP/linux/issues/140

And speaking to the existing code, even if skb->data were
bounds-checked, what are the bounds of "start"?

-- 
Kees Cook