netdev - skb->cb size checks (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] [day] [month] [year] [list]

Date:	Thu, 20 Dec 2012 14:03:40 +0000
From:	David Woodhouse <dwmw2@...radead.org>
To:	David Miller <davem@...emloft.net>
Cc:	netdev@...r.kernel.org
Subject: skb->cb size checks (was Re: [PATCH 00/17] ATM fixes for
 pppoatm/br2684)

On Sat, 2012-12-01 at 20:49 -0500, David Miller wrote:
> From: David Woodhouse <dwmw2@...radead.org>
> Date: Sun, 02 Dec 2012 00:40:47 +0000
> 
> > On Sat, 2012-12-01 at 17:33 +0000, David Woodhouse wrote:
> >> 
> >> Very glad I added the BUILD_BUG_ON on the cb struct size now. Perhaps
> >> there should be a generic helper for that? Something like
> >>  skb_cb_cast(struct foo_cb, skb) could do it automatically...?
> > 
> > Something like this, perhaps? Using skb_cast_cb() would then make it
> > fairly much impossible to accidentally overflow the size of the skb cb.
> 
> 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.

I looked harder at this, and should follow up before it actually does
fall out of the cracks in my brain and get completely forgotten.

Basically, you lie :)

What we *actually* do now, in about two-thirds of cases¹ even in net/
code (I didn't even look at drivers, which I expect to be worse), is use
skb->cb without any form of automatic size check at all. No manual
BUILD_BUG_ON() or anything.

Admittedly, in almost all cases that *isn't* a real problem, because the
structure *isn't* too big for skb->cb and it's all fine. But as a matter
of principle we probably *should* be doing those checks. Just in *case*
someone comes along and adds something stupid to the structure.

So... should we:
 - Ignore the "problem" and leave things as they are.

 - Go through and fix the 2/3 of offending net/ code and then the
   drivers too, *without* making the generic 'deference and automatic
   check' macro that I think would simplify that and help to keep us
   honest in future.
or
 - Let me add something like the skb_cast_cb() macro I wanted, then use
   it in all the offending code I can find.

-- 
dwmw2

¹ http://www.spinics.net/lists/netdev/msg218642.html


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