Re: [PATCH] bpf: btf: Fix a missing check bug

[valdis . kletnieks]

On Mon, 08 Oct 2018 17:44:46 -0700, Song Liu said:

> I think I get the security concept here. However, hdr_len here is only used to
> copy the whole header into kernel space, and it is not used in other
> logic at all.
> I cannot image any security flaw with either hdr_len > btf->hdr->hdr_len case or
> hdr_len < btf->hdr->hdr_len. Could you please provide more insights on what
> would break by malicious user space?

Say the biggest allowed value for hdr_len is 128.  We check the value, the user has 98.
They then stuff 16,383 into there.

Now here's the problem - hdr_len is a local variable, and evaporates when the function
returns.  From here on out, anybody who cares about the header length will use the
value in btf->hdr_len....

(And yes, somebody *does* care about the length, otherwise we wouldn't need a field
saying what the length was....)

Now think how many ways that can go pear-shaped.  You copied in 98 bytes, but outside
the function, they think that header is almost 4 pages long.  Does that ever get used as
a length for kmemcpy()?  Or a limit for a 'for (i=start; i< (start+hdr->hdr_len); i++)' that
walks across a variable length header?

Can you cook up a way to have a good chance to oops the kernel when it walks off the
page you allocated the 98 bytes on?  Can you use it to export chunks of memory out to
userspace?  Lots and lots of ways for this to kersplat a kernel...;

Attachment: pgpAFCnaDZkfi.pgp
Description: PGP signature