Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Tue, 9 Oct 2018 21:47:36 -0700
From:   Alexei Starovoitov <alexei.starovoitov@gmail.com>
To:     Song Liu <liu.song.a23@gmail.com>
Cc:     valdis.kletnieks@vt.edu, wang6495@umn.edu, kjlu@umn.edu,
        Alexei Starovoitov <ast@kernel.org>,
        Daniel Borkmann <daniel@iogearbox.net>,
        Networking <netdev@vger.kernel.org>,
        open list <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH] bpf: btf: Fix a missing check bug
Message-ID: <20181010044735.akxel4pjncafpjcf@ast-mbp.dhcp.thefacebook.com>
References: <1538943795-30895-1-git-send-email-wang6495@umn.edu>
 <CAPhsuW5vnVjRS7DWFYPSmq5G-dAFxOrHCAOMkK-iaOoirzMazQ@mail.gmail.com>
 <43898.1539033428@turing-police.cc.vt.edu>
 <CAPhsuW5nPGMOEMKa1UFwgquoUeYiUV8i8nVgWfM6McH2Ri1esg@mail.gmail.com>
 <9337.1539047240@turing-police.cc.vt.edu>
 <CAPhsuW7-Xa=_2BTATEhthWnyfETPv173p_cG2YrcCZ0BOsS9ow@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <CAPhsuW7-Xa=_2BTATEhthWnyfETPv173p_cG2YrcCZ0BOsS9ow@mail.gmail.com>
User-Agent: NeoMutt/20180223
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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.