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

[Song Liu]

On Mon, Oct 8, 2018 at 6:07 PM <valdis.kletnieks@xxxxxx> wrote:
>
> 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...;

In current code, I don't thing any malicious hdr_len value could pass
btf_check_sec_info().
On the other hand, I agree this is a good-to-have check.

Acked-by: Song Liu <songliubraving@xxxxxx>