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

[Alexei Starovoitov]

On Mon, Oct 08, 2018 at 11:55:15PM -0700, Song Liu wrote:
> 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>

I agree with Song's analysis that the current shape of code in
btf_check_sec_info() has us covered, but it's a good coding style
to check for TOCTOU like this, hence applied to bpf-next.