netdev - Checking struct size against sizeof(skb->cb) (was Re: [PATCH 00/17] ATM fixes for pppoatm/br2684)

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: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <1354483781.24281.16.camel@shinybook.infradead.org>
Date:	Sun, 02 Dec 2012 21:29:41 +0000
From:	David Woodhouse <dwmw2@...radead.org>
To:	David Miller <davem@...emloft.net>
Cc:	netdev@...r.kernel.org
Subject: Checking struct size against sizeof(skb->cb) (was Re: [PATCH 00/17]
 ATM fixes for pppoatm/br2684)

On Sun, 2012-12-02 at 08:14 +0000, David Woodhouse wrote:
> On Sat, 2012-12-01 at 20:49 -0500, David Miller wrote:
> > 
> > I actually prefer what we do now, which is do the BUILD_BUG_ON()
> > once in the subsystem specific code, usually the initializer.
> > 
> > It's part of creating a new SKB cb, adding that assertion somewhere.
> 
> Where it's *subsystem* code that's great...

Hmm... or maybe not. A quick check suggests that about two-thirds of
users even in net/ aren't actually doing the check. My brief
investigation gives a score of 30 to 14 (or thereabouts; there may be
some that *do* check, but I failed to spot it. And I used a Fedora
config, not allyesconfig).

If you don't want an automatic check/cast macro (which is only adding a
compile-time check; no runtime overhead), then is it worth doing a
bombing run on the offenders listed below and adding the manual checks?
And I'll look at *drivers* next... which I suspect will be worse.

I concede there are probably no actual *bugs* being hidden here — I
don't think any of them actually *do* overflow. But since the check is
free at run-time, we *ought* to be doing it. Even if a given struct is
tiny and there's no *chance* of it overflowing, people might still add
to it. After all, the solos_skb_cb struct was tiny too until I stupidly
added a completion to it.

(Actually, I'm not entirely sure about 'no bugs'. The L2TP thing with
starting its own struct at &skb->cb[sizeof(struct inet_skb_parm)]
doesn't make it overflow, but what the hell is l2tp_xmit_skb() doing
poking at IPCB(skb) anyway... is it even guaranteed to be Legacy IP? Can
it not be IPv6? And why would anything else *trust* the contents of ->cb
on a skb that just got handed to it?)

My list:

Size not checked against sizeof(skb->cb):

struct napi_gro_cb (include/linux/netdevice.h)
struct ip6_mtuinfo (include/linux/ipv6.h)
struct sctp_ulpevent (include/net/sctp/ulpevent.h)
struct bt_skb_cb (include/net/bluetooth/bluetooth.h)
struct atm_skb_data (include/linux/atmdev.h)
struct br_input_skb_cb (net/bridge/br_private.h)
struct hci_cb (include/net/bluetooth/hci_core.h)
struct sock_exterr_skb (include/linux/errqueue.h)
struct udp_skb_cb (include/net/udp.h)
struct ipx_cb (include/net/ipx.h)
struct neighbour_cb (include/net/neighbour.h)
struct ip6frag_skb_cb (net/ipv6/reassembly.c)
struct dev_gso_cb (net/core/dev.c)
struct irda_skb_cb (include/net/irda/irda_device.h)
struct cmtp_skb (net/bluetooth/cmtp/cmtp.h)
struct ipfrag_skb_cb (net/ipv6/ip_fragment.c)
struct xfrm_skb_cb (include/net/xfrm.h)
struct xfrm_mode_skb_cb (include/net/xfrm.h)
struct xfrm_spi_skb_cb (include/net/xfrm.h)
struct in_pktinfo (include/uapi/linux/in.h)
struct nf_ct_frag6_skb_cb (net/ipv6/netfilter/nf_conntrack_reasm.c)
struct l2tp_skb_cb (net/l2tp/l2tp_core.c) (less than full cb)
struct ah_skb_cb (net/ipv4/ah4.c)
struct ah_skb_cb (net/ipv6/ah6.c)
struct esp_skb_cb (net/ipv4/esp4.c)
struct esp_skb_cb (net/ipv6/esp6.c)
struct skb_eosp_msg_data (net/mac80211/ieee80211_i.h)
struct mISDNhead (include/linux/mISDNif.h)
struct ieee80211_ra_tid (net/mac80211/iface.c)
struct sctp_input_cb (net/sctp/input.c)

Checked:

struct unix_skb_parms (include/net/af_unix.h)
struct packet_skb_cb (net/packet/af_packet.c)
struct ovs_skb_cb (net/openvswitch/datapath.h)
struct ieee80211_tx_info (include/net/mac80211.h)
struct ieee80211_rx_status (include/net/mac80211.h)
struct tcp_skb_cb (include/net/tcp.h)
struct ieee802154_mac_cb (include/net/ieee802154_netdev.h)
struct netlink_cb_parms (include/linux/netlink.h)
struct inet6_skb_parm (include/linux/ipv6.h)
struct inet_skb_parm (include/net/ip.h)
struct tcp_skb_cb (include/net/tcp.h)
struct garp_skb_cb (include/net/garp.h)
struct dccp_skb_cb (net/dccp/dccp.h)
struct qdisc_skb_cb (include/net/sch_generic.h)

-- 
David Woodhouse                            Open Source Technology Centre
David.Woodhouse@...el.com                              Intel Corporation

Download attachment "smime.p7s" of type "application/x-pkcs7-signature" (6171 bytes)