Messages in this thread | | | Date | Tue, 9 Oct 2018 21:47:36 -0700 | From | Alexei Starovoitov <> | Subject | Re: [PATCH] bpf: btf: Fix a missing check bug |
| |
On Mon, Oct 08, 2018 at 11:55:15PM -0700, Song Liu wrote: > 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>
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.
| |