Replace open-coded skb->data casts with a bounds checking struct-size-aware API

[kees]

The skb interfaces (e.g. skb_put_data()) have internal details on allocation sizes. These should be put to greater use to avoid overflows/underflows.

e.g. https://lore.kernel.org/linux-arm-msm/20210819195034.632132-1-butterflyhuangxx@gmail.com/ could these flaws be better mitigated via changes to the skb API?

See also issue #169

[kees]

skb_put and skb_push both appear to already perform buffer bounds checking. The dangerous code pattern is the open-coded casting to a structure from the raw data pointer:

cf = (struct can_frame *)skb->data;

Instead, there needs to be a bounds-check accessor, more like:

#define skb_peek_data(instance) ({if (sizeof(*instance) > skb->len) skb_over_panic(skb, sizeof(*instance), __builtin_return_address(0); cf = (typeof(*instance) *)skb->data; })

skb_peek_data(cf);

[kees]

@kuba-moo

[kees]

$ git grep ' = .*)skb->data' | grep -vE '^(tools|Documentation|samples)/' | wc -l
1461

[kuba-moo]

Hm, yes, a lot of layers of the stack take skb->data and then validate whether it points to a valid header of their protocol. Or just assume that they are half way thru processing of their layer so the input must have validated already. Tricky stuff, look at ea1627c for instance at how much people care about saving cycles :s
This reminds me of another pitfall with skb data pointers. If you call a helper which may change the geometry of the skb (e.g. pskb_may_pull) you need to reload any data pointers you may have gotten earlier, 'cause the underlying packet may have gotten reallocated.
But I do like the idea of skb_peek_data(), it may not apply everywhere but it should help the noobs DTRT for sure.