Commit 13c164b1a186 - regression for LSMs/SELinux?

[Ian Kent]

On Tue, 2020-09-29 at 14:16 +0200, Ondrej Mosnacek wrote:
> On Sun, Sep 27, 2020 at 5:08 AM Ian Kent <raven at themaw.net> wrote:
> > On Fri, 2020-09-25 at 10:38 -0700, Linus Torvalds wrote:
> > > On Fri, Sep 25, 2020 at 6:38 AM Ondrej Mosnacek <
> > > omosnace at redhat.com>
> > > wrote:
> > > > On Thu, Sep 24, 2020 at 4:16 PM Stephen Smalley
> > > > <stephen.smalley.work at gmail.com> 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.

Ok, let me have a look at the file position handling there.

Ian