Messages in this thread | | | From | Song Liu <> | Date | Mon, 8 Oct 2018 23:55:15 -0700 | Subject | Re: [PATCH] bpf: btf: Fix a missing check bug |
| |
On Mon, Oct 8, 2018 at 6:07 PM <valdis.kletnieks@vt.edu> 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@fb.com>
| |