Re: Commit 13c164b1a186 - regression for LSMs/SELinux?

On Sun, Sep 27, 2020 at 5:08 AM Ian Kent <[email protected]> wrote:
>
> On Fri, 2020-09-25 at 10:38 -0700, Linus Torvalds wrote:
> > On Fri, Sep 25, 2020 at 6:38 AM Ondrej Mosnacek <[email protected]>
> > wrote:
> > > On Thu, Sep 24, 2020 at 4:16 PM Stephen Smalley
> > > <[email protected]> wrote:
> > > > Up-thread I thought Linus indicated he didn't really want a flag
> > > > to
> > > > disable pemission checking due to potential abuse (and I agree).
> > >
> > > IIUC he was against adding an FMODE flag, while I was rather
> > > suggesting a new function parameter (I realize it probably wasn't
> > > clear from what I wrote).
> >
> > I really would prefer neither.
> >
> > Any kind of dynamic behavior that depends on a flag is generally
> > worse
> > than something that can be statically seen.
> >
> > Now, if the flag is _purely_ a constant argument in every single
> > user,
> > and there's no complex flow through multiple different layers, an
> > argument flag is certainly fairly close to just having two different
> > functions for two different behaviors.
> >
> > But I don't really see much of an advantage to adding a new argument
> > to kernel_write() for this - because absolutely *nobody* should ever
> > use it apart from this very special autofs case.
> >
> > So I'd rather just re-export the old __kernel_write() (or whatever it
> > was that broke autofs) that didn't do that particular check. We
> > already use it for splice and core dumping.
> >
> > autofs isn't that different from those two, and I think the only real
> > difference is that autofs is a module. No?
>
> It can be, yes, many distro builds compile it in.
>
> >
> > So I think the fix is as simple as exporting __kernel_write() again -
> > and let's just make it a GPL-only export since we really don't want
> > anybody to use it - and revert  commit 13c164b1a186 ("autofs: switch
> > to kernel_write").
>
> Yes, sorry I missed this initially.
>
> There are a couple of other sanity checks in kern_write() but since
> __kern_write() is meant to be for internal use that's not really
> an issue IMHO.

OK, so it seems that reverting comes out as the best choice here.

BTW, I'm looking at rw_verify_area() and I see this "If (ppos)" check
and the comment above it... And then I look at autofs_write(), which
passes &file->f_pos, while ksys_write() passes file_ppos(file), which
checks FMODE_STREAM and returns NULL if it is set. And since the
autofs pipe should be a... well... pipe, which AFAIK implies
FMODE_STREAM, file_ppos() should return NULL for it. So shouldn't
autofs_write() use file_ppos(file) instead of &file->f_pos? Not sure
if there are any practical implications, but seems more
correct/consistent that way... And in that case most of the
rw_verify_area() checks would be skipped anyway. And
file_start_write()/file_end_write() do nothing on non-regular files,
so it seems kernel_write() vs. __kernel_write() makes only very little
difference for pipes.

--
Ondrej Mosnacek
Software Engineer, Platform Security - SELinux kernel
Red Hat, Inc.