KASAN poisoning for skb linear data

KASAN poisoning for skb linear data

[Dmitry Vyukov]

Hi,

As far as I understand pskb_may_pull() plays important role in packet
parsing for all protocols. And we did custom fragmentation of packets
emitted via tun (IFF_NAPI_FRAGS). However, it seems that it does not
give any results (bugs found), and I think the reason for this is that
linear data is rounded up and is usually quite large. So if a parsing
function does pskb_may_pull(1), or does not do it at all, it can
usually access more and it will go unnoticed. KASAN has an ability to
do custom poisoning: it can poison/unpoison any memory range, and then
detect any reads/writes to that range. What do you think about adding
custom KASAN poisoning to pskb_may_pull() and switching it to
non-eager mode (pull only what was requested) under KASAN? Do you
think it has potential for finding important bugs? What amount of work
is this?

Thanks

Re: KASAN poisoning for skb linear data

[Dmitry Vyukov]

On Mon, Jan 15, 2018 at 3:15 PM, Dmitry Vyukov <dvyukov@google.com> wrote:
> Hi,
>
> As far as I understand pskb_may_pull() plays important role in packet
> parsing for all protocols. And we did custom fragmentation of packets
> emitted via tun (IFF_NAPI_FRAGS). However, it seems that it does not
> give any results (bugs found), and I think the reason for this is that
> linear data is rounded up and is usually quite large. So if a parsing
> function does pskb_may_pull(1), or does not do it at all, it can
> usually access more and it will go unnoticed. KASAN has an ability to
> do custom poisoning: it can poison/unpoison any memory range, and then
> detect any reads/writes to that range. What do you think about adding
> custom KASAN poisoning to pskb_may_pull() and switching it to
> non-eager mode (pull only what was requested) under KASAN? Do you
> think it has potential for finding important bugs? What amount of work
> is this?

Filed https://bugzilla.kernel.org/show_bug.cgi?id=199055 for this so
it's not get lost.

Re: KASAN poisoning for skb linear data

[Stephen Hemminger]

On Thu, 8 Mar 2018 10:20:44 +0100
Dmitry Vyukov <dvyukov@google.com> wrote:

> On Mon, Jan 15, 2018 at 3:15 PM, Dmitry Vyukov <dvyukov@google.com> wrote:
> > Hi,
> >
> > As far as I understand pskb_may_pull() plays important role in packet
> > parsing for all protocols. And we did custom fragmentation of packets
> > emitted via tun (IFF_NAPI_FRAGS). However, it seems that it does not
> > give any results (bugs found), and I think the reason for this is that
> > linear data is rounded up and is usually quite large. So if a parsing
> > function does pskb_may_pull(1), or does not do it at all, it can
> > usually access more and it will go unnoticed. KASAN has an ability to
> > do custom poisoning: it can poison/unpoison any memory range, and then
> > detect any reads/writes to that range. What do you think about adding
> > custom KASAN poisoning to pskb_may_pull() and switching it to
> > non-eager mode (pull only what was requested) under KASAN? Do you
> > think it has potential for finding important bugs? What amount of work
> > is this?  
> 
> Filed https://bugzilla.kernel.org/show_bug.cgi?id=199055 for this so
> it's not get lost.

Linux kernel networking does really use kernel bugzilla.
It is a conduit for bug reports but not used for managing most issues.

Re: KASAN poisoning for skb linear data

[Stephen Hemminger]

On Mon, 15 Jan 2018 15:15:04 +0100
Dmitry Vyukov <dvyukov@google.com> wrote:

> Hi,
> 
> As far as I understand pskb_may_pull() plays important role in packet
> parsing for all protocols. And we did custom fragmentation of packets
> emitted via tun (IFF_NAPI_FRAGS). However, it seems that it does not
> give any results (bugs found), and I think the reason for this is that
> linear data is rounded up and is usually quite large. So if a parsing
> function does pskb_may_pull(1), or does not do it at all, it can
> usually access more and it will go unnoticed. KASAN has an ability to
> do custom poisoning: it can poison/unpoison any memory range, and then
> detect any reads/writes to that range. What do you think about adding
> custom KASAN poisoning to pskb_may_pull() and switching it to
> non-eager mode (pull only what was requested) under KASAN? Do you
> think it has potential for finding important bugs? What amount of work
> is this?
> 
> Thanks

Also, kernel networking only deals with in-tree upstream code.
Any problems with infrastructure for custom code are your problem
to deal with, not our problem.

Re: KASAN poisoning for skb linear data

[Dmitry Vyukov]

On Thu, Mar 8, 2018 at 4:45 PM, Stephen Hemminger
<stephen@networkplumber.org> wrote:
> On Mon, 15 Jan 2018 15:15:04 +0100
> Dmitry Vyukov <dvyukov@google.com> wrote:
>
>> Hi,
>>
>> As far as I understand pskb_may_pull() plays important role in packet
>> parsing for all protocols. And we did custom fragmentation of packets
>> emitted via tun (IFF_NAPI_FRAGS). However, it seems that it does not
>> give any results (bugs found), and I think the reason for this is that
>> linear data is rounded up and is usually quite large. So if a parsing
>> function does pskb_may_pull(1), or does not do it at all, it can
>> usually access more and it will go unnoticed. KASAN has an ability to
>> do custom poisoning: it can poison/unpoison any memory range, and then
>> detect any reads/writes to that range. What do you think about adding
>> custom KASAN poisoning to pskb_may_pull() and switching it to
>> non-eager mode (pull only what was requested) under KASAN? Do you
>> think it has potential for finding important bugs? What amount of work
>> is this?
>>
>> Thanks
>
> Also, kernel networking only deals with in-tree upstream code.
> Any problems with infrastructure for custom code are your problem
> to deal with, not our problem.


This is 100% about upstream code. The issue for me, it is not in
networking component.
I've filed an issue because kernel mailing lists are used mostly for
patches, everything else is immediately lost (e.g. like this one).

Re: KASAN poisoning for skb linear data

[Dmitry Vyukov]

On Thu, Mar 8, 2018 at 1:20 AM, Dmitry Vyukov <dvyukov@google.com> wrote:
> On Mon, Jan 15, 2018 at 3:15 PM, Dmitry Vyukov <dvyukov@google.com> wrote:
>> Hi,
>>
>> As far as I understand pskb_may_pull() plays important role in packet
>> parsing for all protocols. And we did custom fragmentation of packets
>> emitted via tun (IFF_NAPI_FRAGS). However, it seems that it does not
>> give any results (bugs found), and I think the reason for this is that
>> linear data is rounded up and is usually quite large. So if a parsing
>> function does pskb_may_pull(1), or does not do it at all, it can
>> usually access more and it will go unnoticed. KASAN has an ability to
>> do custom poisoning: it can poison/unpoison any memory range, and then
>> detect any reads/writes to that range. What do you think about adding
>> custom KASAN poisoning to pskb_may_pull() and switching it to
>> non-eager mode (pull only what was requested) under KASAN? Do you
>> think it has potential for finding important bugs? What amount of work
>> is this?
>
> Filed https://bugzilla.kernel.org/show_bug.cgi?id=199055 for this so
> it's not get lost.

Bringing this up after we discussed this with Dave on plumbers.

There are 2 strategies for making KASAN aware of exact skb linear
buffer semantics.
1. Just using kmalloc/free each time with precise size.
2. Using KASAN annotations:

void kasan_poison_shadow(const void *address, size_t size, u8 value);
void kasan_unpoison_shadow(const void *address, size_t size);

https://github.com/torvalds/linux/blob/master/mm/kasan/kasan.c#L57
https://github.com/torvalds/linux/blob/master/include/linux/kasan.h#L38

If we use annotations we can keep more of the existing skb logic.
But AFAIU this way we won't be able to detect all accesses after a
potential reallocation.

There are also annotations for explicit checks:
https://github.com/torvalds/linux/blob/master/include/linux/kasan-checks.h#L6
But not sure we need them here (maybe more appropriate for places
where KASAN does not see memory accesses e.g. a driver handing off a
packet to DMA).


I don't think it makes sense to make any more complex than necessary
in the name of performance, at least initially. This will be enabled
only under #ifdef KASAN.

If somebody gives us any prototype, we can assess (1) if it works and
(2) if it catches any new bugs.

Thanks