On Mon, 18 Jul 2016, One Thousand Gnomes wrote:
> On Mon, 18 Jul 2016 11:45:53 -0400 (EDT)
> Nicolas Pitre <nicolas.pitre@xxxxxxxxxx> wrote:
>
> > On Mon, 18 Jul 2016, One Thousand Gnomes wrote:
> >
> > > On Sun, 17 Jul 2016 23:31:56 -0400
> > > Nicolas Pitre <nicolas.pitre@xxxxxxxxxx> wrote:
> > >
> > > > Let's take the simple and obvious approach by decompressing the binary
> > > > into a kernel buffer and then copying it to user space. Those who are
> > > > looking for more performance on a MMU system are unlikely to choose this
> > > > executable format anyway.
> > >
> > > The flat loader takes a very casual attitude to overruns and corrupted
> > > binaries. It's after all MMUless so has no real security model. If you
> > > enable flat for an MMU system then IMHO those all need to be fixed
> > > including all the missing overflow checks on the maths on textlen and the
> > > like.
> >
> > What about the following patch? This with existing user accessors and
> > allocation error checks should cover it all.
> >
> > ----- >8
> > commit cc1051c9c57202772568600e96b75229a2a7cf19
> > Author: Nicolas Pitre <nicolas.pitre@xxxxxxxxxx>
> > Date: Mon Jul 18 11:28:57 2016 -0400
> >
> > binfmt_flat: prevent kernel dammage from corrupted executable headers
> >
> > Signed-off-by: Nicolas Pitre <nico@xxxxxxxxxx>
> >
> > diff --git a/fs/binfmt_flat.c b/fs/binfmt_flat.c
> > index 24deae4dcb..fa0054c1c3 100644
> > --- a/fs/binfmt_flat.c
> > +++ b/fs/binfmt_flat.c
> > @@ -498,6 +498,17 @@ static int load_flat_file(struct linux_binprm * bprm,
> > }
> >
> > /*
> > + * Make sure the header params are sane.
> > + * 28 bits (256 MB) is way more than reasonable in this case.
> > + * If some top bits are set we have probable binary corruption.
> > + */
> > + if ((text_len | data_len | bss_len | stack_len | full_data) >> 28) {
> > + printk("BINFMT_FLAT: bad header\n");
>
> Apart from the printk that looks good for the header but I think the rest
> could do with a fair bit more review (eg relocations in range checks).
Given that they all go through put_user() now, the worst that could
happen is an executable that craps onto itself. I don't think there is
much we can do here besides letting the user task crash.
Nicolas
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
