Re: [report] OOB in bpf_load_prog() flow

[Kees Cook <keescook@chromium.org>]

On Fri, Dec 23, 2022 at 01:45:51AM -0800, Hyunwoo Kim wrote:
> On Tue, Dec 20, 2022 at 01:03:47PM -0800, Stanislav Fomichev wrote:
> > On Tue, Dec 20, 2022 at 11:08 AM Kees Cook <kees@kernel.org> wrote:
> > >
> > > On December 20, 2022 9:32:51 AM PST, Stanislav Fomichev <sdf@google.com> wrote:
> > > >On Tue, Dec 20, 2022 at 3:37 AM Hyunwoo Kim <v4bel@theori.io> wrote:
> > > >>
> > > >> On Mon, Dec 19, 2022 at 11:02:32AM -0800, sdf@google.com wrote:
> > > >> > On 12/19, Hyunwoo Kim wrote:
> > > >> > > Dear,
> > > >> >
> > > >> > > This slab-out-of-bounds occurs in the bpf_prog_load() flow:
> > > >> > > https://syzkaller.appspot.com/text?tag=CrashLog&x=172e2510480000
> > > >> >
> > > >> > > I was able to trigger KASAN using this syz reproduce code:
> > > [...]
> > > >> >
> > > >> > > IMHO, the root cause of this seems to be commit
> > > >> > > ceb35b666d42c2e91b1f94aeca95bb5eb0943268.
> > > >> >
> > > >> > > Also, a user with permission to load a BPF program can use this OOB to
> > > >> > > execute the desired code with kernel privileges.
> > > >> >
> > > >> > Let's CC Kees if you suspect the commit above. Maybe we can run
> > > >> > with/without it to confirm?
> > > >>
> > > >> I built and tested each commit of 'kernel/bpf/verifier.c' that caused
> > > >> OOB, but I couldn't find the commit that caused OOB.
> > > >>
> > > >> So, starting from upstream, I reversed commits one by one and
> > > >> found the commit that triggers KASAN.
> > > >>
> > > >> As a result of testing, OOB is triggered from commit
> > > >> 8fa590bf344816c925810331eea8387627bbeb40.
> > > >>
> > > >> However, this commit seems to be a kvm related patch,
> > > >> not directly related to the bpf subsystem.
> > > >>
> > > >> IMHO, the cause of this seems to be one of these:
> > > >> 1. I ran this KASAN test on a nested guest in L2. That is,
> > > >> there is a problem with the kvm patch 8fa590bf34481.
> > > >>
> > > >> 2. Previously, the BPF subsystem had a patch that triggers KASAN,
> > > >> and KASAN is induced when kvm is patched.
> > > >>
> > > >> 3. There was confusion in the .config I tested, so the wrong
> > > >> patch was derived as a test result.
> > > >>
> > > >> I haven't been able to pinpoint what the root cause is yet.
> > > >> So I didn't add a CC for 8fa590bf34481 commit.
> > > >
> > > >Thanks for the details! Even if this particular one is unrelated,
> > > >there are a couple of reports which still somewhat look like they are
> > > >related to commit ceb35b666d42 ("bpf/verifier: Use
> > > >kmalloc_size_roundup() to match ksize() usage") ?
> > > >
> > > >https://lore.kernel.org/bpf/000000000000ab724705ee87e321@google.com/
> > > >https://lore.kernel.org/bpf/000000000000269f9a05f02be9d8@google.com/
> > >
> > > I suspect something is hitting array_resize() that wasn't maximal-bucket-size allocated. Does reverting 38931d8989b5760b0bd17c9ec99e81986258e4cb make it go away?
> > 
> > Reverting makes it go away for at least one of them:
> > https://lore.kernel.org/bpf/0000000000004bee2205f0484e1d@google.com/T/#m60dba18e94e01094a899ab7fe8d19aa1a3cf26fe
> > 
> > The second one didn't like my patch, I'm trying again now:
> > https://lore.kernel.org/bpf/Y6Iipad5vz55tl2A@google.com/T/#m032bed8c3d47f33a9fccd660446beabce98ff5fe
> 
> I have found the root cause of this issue.
> 
> This happens when krealloc() in push_jmp_history() receives an allocation 
> request with a size smaller than the existing slab object.
> Based on the poc code I first reported, it occurs when cur->jmp_history 
> of 64 bytes is krealloc()ed to 32 bytes.
> 
> When krealloc() is called with a smaller size than the previous one as an argument, 
> krealloc() does not 'kfree&reallocate', but only resets the slab redzone based on the newly received size (if KASAN is activated).
> That's why `ksize(dst)` in copy_array() afterward returns the actual allocation size of 64 bytes. 
> However, KASAN recognizes memcpy(dst, src, 64) as OOB because the redzone is set after 32 bytes by krealloc().
> 
> In any case, since the actually allocated size of dst is 64 bytes, exploiting this is close to impossible.
> However, it seems that copy_array() needs to be patched in the direction of not using ksize().

Ah-ha, thanks for tracking it down. I think this should fix it:
https://lore.kernel.org/bpf/20221223182836.never.866-kees@kernel.org

-- 
Kees Cook