Messages in this thread | | | From | Dmitry Vyukov <> | Date | Fri, 16 Nov 2018 16:52:46 -0800 | Subject | Re: KASAN poisoning for skb linear data |
| |
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
| |