Commit 13c164b1a186 - regression for LSMs/SELinux?

Commit 13c164b1a186 - regression for LSMs/SELinux?

[Ondrej Mosnacek]

Hi folks,

It seems that after commit 13c164b1a186 ("autofs: switch to
kernel_write") there is now an extra LSM permission required (for the
current task to write to the automount pipe) for processes accessing
some yet-to-to-be mounted directory on which an autofs mount is set
up. The call chain is:
[...]
autofs_wait() ->
autofs_notify_daemon() ->
autofs_write() ->
kernel_write() ->
rw_verify_area() ->
security_file_permission()

The bug report that led me to this commit is at [1].

Technically, this is a regression for LSM users, since this is a
kernel-internal operation and an LSM permission for the current task
shouldn't be required. Can this patch be reverted? Perhaps
__kernel_{read|write}() could instead be renamed to kernel_*_nocheck()
so that the name is more descriptive?

Thanks,

[1] https://bugzilla.redhat.com/show_bug.cgi?id=1874338

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

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

[Christoph Hellwig]

[adding Linus and Al]

On Mon, Sep 21, 2020 at 04:51:35PM +0200, Ondrej Mosnacek wrote:
> Hi folks,
> 
> It seems that after commit 13c164b1a186 ("autofs: switch to
> kernel_write") there is now an extra LSM permission required (for the
> current task to write to the automount pipe) for processes accessing
> some yet-to-to-be mounted directory on which an autofs mount is set
> up. The call chain is:
> [...]
> autofs_wait() ->
> autofs_notify_daemon() ->
> autofs_write() ->
> kernel_write() ->
> rw_verify_area() ->
> security_file_permission()
> 
> The bug report that led me to this commit is at [1].
> 
> Technically, this is a regression for LSM users, since this is a
> kernel-internal operation and an LSM permission for the current task
> shouldn't be required. Can this patch be reverted? Perhaps
> __kernel_{read|write}() could instead be renamed to kernel_*_nocheck()
> so that the name is more descriptive?

So we obviously should not break existing user space and need to fix
this ASAP.  The trivial "fix" would be to export __kernel_write again
and switch autofs to use it.  The other option would be a FMODE flag
to bypass security checks, only to be set if the callers ensures
they've been valided (i.e. in autofs_prepare_pipe).

Any opinions?

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

[Linus Torvalds]

On Mon, Sep 21, 2020 at 9:09 AM Christoph Hellwig <hch@lst.de> wrote:
>
> So we obviously should not break existing user space and need to fix
> this ASAP.  The trivial "fix" would be to export __kernel_write again
> and switch autofs to use it.  The other option would be a FMODE flag
> to bypass security checks, only to be set if the callers ensures
> they've been valided (i.e. in autofs_prepare_pipe).
>
> Any opinions?

I'd much rather do the former than add a new dynamic flag that we then
have to worry about somebody being able to set thanks to a bug.

Static behavior is a lot easier to verify and document (ie just a
comment in the code explaining why autofs cannot use the regular
kernel_write()). There's no chance of that static behavior then
leaking to other call sites.

                   Linus

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

[Al Viro]

On Mon, Sep 21, 2020 at 06:09:22PM +0200, Christoph Hellwig wrote:
> [adding Linus and Al]
> 
> On Mon, Sep 21, 2020 at 04:51:35PM +0200, Ondrej Mosnacek wrote:
> > Hi folks,
> > 
> > It seems that after commit 13c164b1a186 ("autofs: switch to
> > kernel_write") there is now an extra LSM permission required (for the
> > current task to write to the automount pipe) for processes accessing
> > some yet-to-to-be mounted directory on which an autofs mount is set
> > up. The call chain is:
> > [...]
> > autofs_wait() ->
> > autofs_notify_daemon() ->
> > autofs_write() ->
> > kernel_write() ->
> > rw_verify_area() ->
> > security_file_permission()
> > 
> > The bug report that led me to this commit is at [1].
> > 
> > Technically, this is a regression for LSM users, since this is a
> > kernel-internal operation and an LSM permission for the current task
> > shouldn't be required. Can this patch be reverted? Perhaps
> > __kernel_{read|write}() could instead be renamed to kernel_*_nocheck()
> > so that the name is more descriptive?
> 
> So we obviously should not break existing user space and need to fix
> this ASAP.  The trivial "fix" would be to export __kernel_write again
> and switch autofs to use it.  The other option would be a FMODE flag
> to bypass security checks, only to be set if the callers ensures
> they've been valided (i.e. in autofs_prepare_pipe).
> 
> Any opinions?

Reexport for now.  Incidentally, what is LSM doing rejecting writes
into a pipe?

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

[Ian Kent]

On Mon, 2020-09-21 at 17:30 +0100, Al Viro wrote:
> On Mon, Sep 21, 2020 at 06:09:22PM +0200, Christoph Hellwig wrote:
> > [adding Linus and Al]
> > 
> > On Mon, Sep 21, 2020 at 04:51:35PM +0200, Ondrej Mosnacek wrote:
> > > Hi folks,
> > > 
> > > It seems that after commit 13c164b1a186 ("autofs: switch to
> > > kernel_write") there is now an extra LSM permission required (for
> > > the
> > > current task to write to the automount pipe) for processes
> > > accessing
> > > some yet-to-to-be mounted directory on which an autofs mount is
> > > set
> > > up. The call chain is:
> > > [...]
> > > autofs_wait() ->
> > > autofs_notify_daemon() ->
> > > autofs_write() ->
> > > kernel_write() ->
> > > rw_verify_area() ->
> > > security_file_permission()
> > > 
> > > The bug report that led me to this commit is at [1].
> > > 
> > > Technically, this is a regression for LSM users, since this is a
> > > kernel-internal operation and an LSM permission for the current
> > > task
> > > shouldn't be required. Can this patch be reverted? Perhaps
> > > __kernel_{read|write}() could instead be renamed to
> > > kernel_*_nocheck()
> > > so that the name is more descriptive?
> > 
> > So we obviously should not break existing user space and need to
> > fix
> > this ASAP.  The trivial "fix" would be to export __kernel_write
> > again
> > and switch autofs to use it.  The other option would be a FMODE
> > flag
> > to bypass security checks, only to be set if the callers ensures
> > they've been valided (i.e. in autofs_prepare_pipe).
> > 
> > Any opinions?
> 
> Reexport for now.  Incidentally, what is LSM doing rejecting writes
> into a pipe?

I had seen this too but thought it was due to selinux policy changes
but the previously linked bug shows the rejection is more widespread
than I thought.

A revert seems sensible for now but I'd like to understand why the
writes are being rejected too, I'll have a look around.

Ian

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

[Ian Kent]

On Tue, 2020-09-22 at 08:30 +0800, Ian Kent wrote:
> On Mon, 2020-09-21 at 17:30 +0100, Al Viro wrote:
> > On Mon, Sep 21, 2020 at 06:09:22PM +0200, Christoph Hellwig wrote:
> > > [adding Linus and Al]
> > > 
> > > On Mon, Sep 21, 2020 at 04:51:35PM +0200, Ondrej Mosnacek wrote:
> > > > Hi folks,
> > > > 
> > > > It seems that after commit 13c164b1a186 ("autofs: switch to
> > > > kernel_write") there is now an extra LSM permission required
> > > > (for
> > > > the
> > > > current task to write to the automount pipe) for processes
> > > > accessing
> > > > some yet-to-to-be mounted directory on which an autofs mount is
> > > > set
> > > > up. The call chain is:
> > > > [...]
> > > > autofs_wait() ->
> > > > autofs_notify_daemon() ->
> > > > autofs_write() ->
> > > > kernel_write() ->
> > > > rw_verify_area() ->
> > > > security_file_permission()
> > > > 
> > > > The bug report that led me to this commit is at [1].
> > > > 
> > > > Technically, this is a regression for LSM users, since this is
> > > > a
> > > > kernel-internal operation and an LSM permission for the current
> > > > task
> > > > shouldn't be required. Can this patch be reverted? Perhaps
> > > > __kernel_{read|write}() could instead be renamed to
> > > > kernel_*_nocheck()
> > > > so that the name is more descriptive?
> > > 
> > > So we obviously should not break existing user space and need to
> > > fix
> > > this ASAP.  The trivial "fix" would be to export __kernel_write
> > > again
> > > and switch autofs to use it.  The other option would be a FMODE
> > > flag
> > > to bypass security checks, only to be set if the callers ensures
> > > they've been valided (i.e. in autofs_prepare_pipe).
> > > 
> > > Any opinions?
> > 
> > Reexport for now.  Incidentally, what is LSM doing rejecting writes
> > into a pipe?
> 
> I had seen this too but thought it was due to selinux policy changes
> but the previously linked bug shows the rejection is more widespread
> than I thought.
> 
> A revert seems sensible for now but I'd like to understand why the
> writes are being rejected too, I'll have a look around.

There's not much to see looking around.

Based on the reports it appears that the added security check denies
processes other than the pipe creator write access to the pipe but
in order to trigger an automount pretty much any process needs to be
allowed to write to the automount owned kernel communication pipe.

So I still think it's an selinux policy problem.

Ian

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

[Ondrej Mosnacek]

On Mon, Sep 21, 2020 at 6:30 PM Al Viro <viro@zeniv.linux.org.uk> wrote:
> On Mon, Sep 21, 2020 at 06:09:22PM +0200, Christoph Hellwig wrote:
> > [adding Linus and Al]
> >
> > On Mon, Sep 21, 2020 at 04:51:35PM +0200, Ondrej Mosnacek wrote:
> > > Hi folks,
> > >
> > > It seems that after commit 13c164b1a186 ("autofs: switch to
> > > kernel_write") there is now an extra LSM permission required (for the
> > > current task to write to the automount pipe) for processes accessing
> > > some yet-to-to-be mounted directory on which an autofs mount is set
> > > up. The call chain is:
> > > [...]
> > > autofs_wait() ->
> > > autofs_notify_daemon() ->
> > > autofs_write() ->
> > > kernel_write() ->
> > > rw_verify_area() ->
> > > security_file_permission()
> > >
> > > The bug report that led me to this commit is at [1].
> > >
> > > Technically, this is a regression for LSM users, since this is a
> > > kernel-internal operation and an LSM permission for the current task
> > > shouldn't be required. Can this patch be reverted? Perhaps
> > > __kernel_{read|write}() could instead be renamed to kernel_*_nocheck()
> > > so that the name is more descriptive?
> >
> > So we obviously should not break existing user space and need to fix
> > this ASAP.  The trivial "fix" would be to export __kernel_write again
> > and switch autofs to use it.  The other option would be a FMODE flag
> > to bypass security checks, only to be set if the callers ensures
> > they've been valided (i.e. in autofs_prepare_pipe).

IMHO that sounds like an overkill in this scenario. I don't think it
makes sense to do the LSM check here (or at least not against the
current task's creds), because it is not the current task that wants
to communicate with the daemon, it just wants to to access some
directory on the system that just happens to be special to the kernel,
which needs to do some communication on the side to service this
request. So if we do want to do any LSM check here, there should at
least be some "bool internal" flag passed to the LSM, signalizing that
this is an internal read/write operation that wasn't directly
initiated/requested by the current process. SELinux could then either
use the kernel secid instead of the current task's secid or skip the
check completely in such case.

I'd like Stephen to weigh in on this, but it looks he might be on
vacation right now...

> >
> > Any opinions?
>
> Reexport for now.  Incidentally, what is LSM doing rejecting writes
> into a pipe?

With SELinux at least, what is allowed or denied is defined in the
policy. And the policy usually defaults to everything denied and then
you add rules to allow what needs (and makes sense) to be allowed.
Since until kernel 5.8 random processes didn't need to write to pipes
created by the automount daemon, it has never been explicitly allowed
and so the automounting now fails. It is in no way obvious that all
processes should have the permission to talk to the automount daemon
just to traverse the filesystem...

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

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

[Stephen Smalley]

On Tue, Sep 22, 2020 at 3:34 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:
>
> On Mon, Sep 21, 2020 at 6:30 PM Al Viro <viro@zeniv.linux.org.uk> wrote:
> > On Mon, Sep 21, 2020 at 06:09:22PM +0200, Christoph Hellwig wrote:
> > > [adding Linus and Al]
> > >
> > > On Mon, Sep 21, 2020 at 04:51:35PM +0200, Ondrej Mosnacek wrote:
> > > > Hi folks,
> > > >
> > > > It seems that after commit 13c164b1a186 ("autofs: switch to
> > > > kernel_write") there is now an extra LSM permission required (for the
> > > > current task to write to the automount pipe) for processes accessing
> > > > some yet-to-to-be mounted directory on which an autofs mount is set
> > > > up. The call chain is:
> > > > [...]
> > > > autofs_wait() ->
> > > > autofs_notify_daemon() ->
> > > > autofs_write() ->
> > > > kernel_write() ->
> > > > rw_verify_area() ->
> > > > security_file_permission()
> > > >
> > > > The bug report that led me to this commit is at [1].
> > > >
> > > > Technically, this is a regression for LSM users, since this is a
> > > > kernel-internal operation and an LSM permission for the current task
> > > > shouldn't be required. Can this patch be reverted? Perhaps
> > > > __kernel_{read|write}() could instead be renamed to kernel_*_nocheck()
> > > > so that the name is more descriptive?
> > >
> > > So we obviously should not break existing user space and need to fix
> > > this ASAP.  The trivial "fix" would be to export __kernel_write again
> > > and switch autofs to use it.  The other option would be a FMODE flag
> > > to bypass security checks, only to be set if the callers ensures
> > > they've been valided (i.e. in autofs_prepare_pipe).
>
> IMHO that sounds like an overkill in this scenario. I don't think it
> makes sense to do the LSM check here (or at least not against the
> current task's creds), because it is not the current task that wants
> to communicate with the daemon, it just wants to to access some
> directory on the system that just happens to be special to the kernel,
> which needs to do some communication on the side to service this
> request. So if we do want to do any LSM check here, there should at
> least be some "bool internal" flag passed to the LSM, signalizing that
> this is an internal read/write operation that wasn't directly
> initiated/requested by the current process. SELinux could then either
> use the kernel secid instead of the current task's secid or skip the
> check completely in such case.
>
> I'd like Stephen to weigh in on this, but it looks he might be on
> vacation right now...

No, just wasn't cc'd previously.  I don't think we want any LSM check
here.  As the long term fix, I would suggest moving the
security_file_permission() call up from rw_verify_area() to the
callers (and not call it from kernel_write() at all).

>
> > >
> > > Any opinions?
> >
> > Reexport for now.  Incidentally, what is LSM doing rejecting writes
> > into a pipe?
>
> With SELinux at least, what is allowed or denied is defined in the
> policy. And the policy usually defaults to everything denied and then
> you add rules to allow what needs (and makes sense) to be allowed.
> Since until kernel 5.8 random processes didn't need to write to pipes
> created by the automount daemon, it has never been explicitly allowed
> and so the automounting now fails. It is in no way obvious that all
> processes should have the permission to talk to the automount daemon
> just to traverse the filesystem...

Yes, controlling pipe writes is just one part of controlling
information flow between processes but the intent here is to control
userspace actions, not kernel-internal operations.

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

[Ian Kent]

On Tue, 2020-09-22 at 09:33 +0200, Ondrej Mosnacek wrote:
> On Mon, Sep 21, 2020 at 6:30 PM Al Viro <viro@zeniv.linux.org.uk>
> wrote:
> > On Mon, Sep 21, 2020 at 06:09:22PM +0200, Christoph Hellwig wrote:
> > > [adding Linus and Al]
> > > 
> > > On Mon, Sep 21, 2020 at 04:51:35PM +0200, Ondrej Mosnacek wrote:
> > > > Hi folks,
> > > > 
> > > > It seems that after commit 13c164b1a186 ("autofs: switch to
> > > > kernel_write") there is now an extra LSM permission required
> > > > (for the
> > > > current task to write to the automount pipe) for processes
> > > > accessing
> > > > some yet-to-to-be mounted directory on which an autofs mount is
> > > > set
> > > > up. The call chain is:
> > > > [...]
> > > > autofs_wait() ->
> > > > autofs_notify_daemon() ->
> > > > autofs_write() ->
> > > > kernel_write() ->
> > > > rw_verify_area() ->
> > > > security_file_permission()
> > > > 
> > > > The bug report that led me to this commit is at [1].
> > > > 
> > > > Technically, this is a regression for LSM users, since this is
> > > > a
> > > > kernel-internal operation and an LSM permission for the current
> > > > task
> > > > shouldn't be required. Can this patch be reverted? Perhaps
> > > > __kernel_{read|write}() could instead be renamed to
> > > > kernel_*_nocheck()
> > > > so that the name is more descriptive?
> > > 
> > > So we obviously should not break existing user space and need to
> > > fix
> > > this ASAP.  The trivial "fix" would be to export __kernel_write
> > > again
> > > and switch autofs to use it.  The other option would be a FMODE
> > > flag
> > > to bypass security checks, only to be set if the callers ensures
> > > they've been valided (i.e. in autofs_prepare_pipe).
> 
> IMHO that sounds like an overkill in this scenario. I don't think it
> makes sense to do the LSM check here (or at least not against the
> current task's creds), because it is not the current task that wants
> to communicate with the daemon, it just wants to to access some
> directory on the system that just happens to be special to the
> kernel,
> which needs to do some communication on the side to service this
> request. So if we do want to do any LSM check here, there should at
> least be some "bool internal" flag passed to the LSM, signalizing
> that
> this is an internal read/write operation that wasn't directly
> initiated/requested by the current process. SELinux could then either
> use the kernel secid instead of the current task's secid or skip the
> check completely in such case.

Perhaps, but see below.

> 
> I'd like Stephen to weigh in on this, but it looks he might be on
> vacation right now...
> 
> > > Any opinions?
> > 
> > Reexport for now.  Incidentally, what is LSM doing rejecting writes
> > into a pipe?
> 
> With SELinux at least, what is allowed or denied is defined in the
> policy. And the policy usually defaults to everything denied and then
> you add rules to allow what needs (and makes sense) to be allowed.
> Since until kernel 5.8 random processes didn't need to write to pipes
> created by the automount daemon, it has never been explicitly allowed
> and so the automounting now fails. It is in no way obvious that all
> processes should have the permission to talk to the automount daemon
> just to traverse the filesystem...

I think you might have misunderstood what lead to this, just a bit.

Previously the __kern_write() function was used for this communication
and Christoph's patch changed that to use kern_write() instead.

In theory that's a good idea because kern_write() adds some additional
sanity checks, one being a call to rw_verify_area() which is where the
security_file_permission() call fails.

So previously any random process could avoid these checks by calling
__kern_write() so the change to kern_write() is, in theory, that's a
good thing and simply reverting that hunk in Christoph's patch
probably isn't the best thing to do.

But any random process does need to be able to write to the automount
daemon pipe for trailing path components and the root dentry of autofs
mounts, depending on case.

So it's true that any write to any autofs dentry probably doesn't
need to be allowed but I question what that gets us in terms of
security improvement over allowing pipe writes for automount_t
labelled pipes in selinux policy since they must be within an autofs
mounted file system.

But Stephen has a different recommendation (and that appears to
consider the cause I outlined above) so I'll wait to see what others
think about the recommendations.

Ian

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

[Ondrej Mosnacek]

On Wed, Sep 23, 2020 at 3:55 AM Ian Kent <raven@themaw.net> wrote:
> On Tue, 2020-09-22 at 09:33 +0200, Ondrej Mosnacek wrote:
> > On Mon, Sep 21, 2020 at 6:30 PM Al Viro <viro@zeniv.linux.org.uk>
> > wrote:
> > > On Mon, Sep 21, 2020 at 06:09:22PM +0200, Christoph Hellwig wrote:
> > > > [adding Linus and Al]
> > > >
> > > > On Mon, Sep 21, 2020 at 04:51:35PM +0200, Ondrej Mosnacek wrote:
> > > > > Hi folks,
> > > > >
> > > > > It seems that after commit 13c164b1a186 ("autofs: switch to
> > > > > kernel_write") there is now an extra LSM permission required
> > > > > (for the
> > > > > current task to write to the automount pipe) for processes
> > > > > accessing
> > > > > some yet-to-to-be mounted directory on which an autofs mount is
> > > > > set
> > > > > up. The call chain is:
> > > > > [...]
> > > > > autofs_wait() ->
> > > > > autofs_notify_daemon() ->
> > > > > autofs_write() ->
> > > > > kernel_write() ->
> > > > > rw_verify_area() ->
> > > > > security_file_permission()
> > > > >
> > > > > The bug report that led me to this commit is at [1].
> > > > >
> > > > > Technically, this is a regression for LSM users, since this is
> > > > > a
> > > > > kernel-internal operation and an LSM permission for the current
> > > > > task
> > > > > shouldn't be required. Can this patch be reverted? Perhaps
> > > > > __kernel_{read|write}() could instead be renamed to
> > > > > kernel_*_nocheck()
> > > > > so that the name is more descriptive?
> > > >
> > > > So we obviously should not break existing user space and need to
> > > > fix
> > > > this ASAP.  The trivial "fix" would be to export __kernel_write
> > > > again
> > > > and switch autofs to use it.  The other option would be a FMODE
> > > > flag
> > > > to bypass security checks, only to be set if the callers ensures
> > > > they've been valided (i.e. in autofs_prepare_pipe).
> >
> > IMHO that sounds like an overkill in this scenario. I don't think it
> > makes sense to do the LSM check here (or at least not against the
> > current task's creds), because it is not the current task that wants
> > to communicate with the daemon, it just wants to to access some
> > directory on the system that just happens to be special to the
> > kernel,
> > which needs to do some communication on the side to service this
> > request. So if we do want to do any LSM check here, there should at
> > least be some "bool internal" flag passed to the LSM, signalizing
> > that
> > this is an internal read/write operation that wasn't directly
> > initiated/requested by the current process. SELinux could then either
> > use the kernel secid instead of the current task's secid or skip the
> > check completely in such case.
>
> Perhaps, but see below.
>
> >
> > I'd like Stephen to weigh in on this, but it looks he might be on
> > vacation right now...
> >
> > > > Any opinions?
> > >
> > > Reexport for now.  Incidentally, what is LSM doing rejecting writes
> > > into a pipe?
> >
> > With SELinux at least, what is allowed or denied is defined in the
> > policy. And the policy usually defaults to everything denied and then
> > you add rules to allow what needs (and makes sense) to be allowed.
> > Since until kernel 5.8 random processes didn't need to write to pipes
> > created by the automount daemon, it has never been explicitly allowed
> > and so the automounting now fails. It is in no way obvious that all
> > processes should have the permission to talk to the automount daemon
> > just to traverse the filesystem...
>
> I think you might have misunderstood what lead to this, just a bit.
>
> Previously the __kern_write() function was used for this communication
> and Christoph's patch changed that to use kern_write() instead.
>
> In theory that's a good idea because kern_write() adds some additional
> sanity checks, one being a call to rw_verify_area() which is where the
> security_file_permission() call fails.
>
> So previously any random process could avoid these checks by calling
> __kern_write() so the change to kern_write() is, in theory, that's a
> good thing and simply reverting that hunk in Christoph's patch
> probably isn't the best thing to do.

I understand that and I'm not proposing the revert as a long-term fix.
For a long-term solution I propose using kernel_write() and extending
it to allow the caller to suppress (just) the
security_file_permission() call. Then each caller would have to decide
whether the LSM check makes sense in that situation or not. It seems
safer against future mistakes than leaving it up to the caller to call
security_file_permission() explicitly (I predict that no new user
would even realize that the call might be needed).

>
> But any random process does need to be able to write to the automount
> daemon pipe for trailing path components and the root dentry of autofs
> mounts, depending on case.
>
> So it's true that any write to any autofs dentry probably doesn't
> need to be allowed but I question what that gets us in terms of
> security improvement over allowing pipe writes for automount_t
> labelled pipes in selinux policy since they must be within an autofs
> mounted file system.

The difference is not in security, but in usability. The kernel
communicating with the autofs daemon is an internal detail that
shouldn't need special rules in policy. Even if we want to do any LSM
checking here, the subject should be kernel_t, not the current running
process. The process doesn't have any control on whether the kernel
does the communication and it doesn't control the content of the
communication, so the permission check as it is doesn't make any
sense. People writing the policy should be burdened by low-level
details about how the kernel works internally as little as possible.

>
> But Stephen has a different recommendation (and that appears to
> consider the cause I outlined above) so I'll wait to see what others
> think about the recommendations.

As I said above, I think Stephen's approach is less future-proof. And
it seems that rw_verify_area() has many other callers, most/all of
which probably service actual requests from userspace and we'd need to
retain the security_file_permission() call in those cases.

Let me try to put my proposal into a patch, so we have something
concrete to talk about...

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

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

[Ian Kent]

On Thu, 2020-09-24 at 10:36 +0200, Ondrej Mosnacek wrote:
> On Wed, Sep 23, 2020 at 3:55 AM Ian Kent <raven@themaw.net> wrote:
> > On Tue, 2020-09-22 at 09:33 +0200, Ondrej Mosnacek wrote:
> > > On Mon, Sep 21, 2020 at 6:30 PM Al Viro <viro@zeniv.linux.org.uk>
> > > wrote:
> > > > On Mon, Sep 21, 2020 at 06:09:22PM +0200, Christoph Hellwig
> > > > wrote:
> > > > > [adding Linus and Al]
> > > > > 
> > > > > On Mon, Sep 21, 2020 at 04:51:35PM +0200, Ondrej Mosnacek
> > > > > wrote:
> > > > > > Hi folks,
> > > > > > 
> > > > > > It seems that after commit 13c164b1a186 ("autofs: switch to
> > > > > > kernel_write") there is now an extra LSM permission
> > > > > > required
> > > > > > (for the
> > > > > > current task to write to the automount pipe) for processes
> > > > > > accessing
> > > > > > some yet-to-to-be mounted directory on which an autofs
> > > > > > mount is
> > > > > > set
> > > > > > up. The call chain is:
> > > > > > [...]
> > > > > > autofs_wait() ->
> > > > > > autofs_notify_daemon() ->
> > > > > > autofs_write() ->
> > > > > > kernel_write() ->
> > > > > > rw_verify_area() ->
> > > > > > security_file_permission()
> > > > > > 
> > > > > > The bug report that led me to this commit is at [1].
> > > > > > 
> > > > > > Technically, this is a regression for LSM users, since this
> > > > > > is
> > > > > > a
> > > > > > kernel-internal operation and an LSM permission for the
> > > > > > current
> > > > > > task
> > > > > > shouldn't be required. Can this patch be reverted? Perhaps
> > > > > > __kernel_{read|write}() could instead be renamed to
> > > > > > kernel_*_nocheck()
> > > > > > so that the name is more descriptive?
> > > > > 
> > > > > So we obviously should not break existing user space and need
> > > > > to
> > > > > fix
> > > > > this ASAP.  The trivial "fix" would be to export
> > > > > __kernel_write
> > > > > again
> > > > > and switch autofs to use it.  The other option would be a
> > > > > FMODE
> > > > > flag
> > > > > to bypass security checks, only to be set if the callers
> > > > > ensures
> > > > > they've been valided (i.e. in autofs_prepare_pipe).
> > > 
> > > IMHO that sounds like an overkill in this scenario. I don't think
> > > it
> > > makes sense to do the LSM check here (or at least not against the
> > > current task's creds), because it is not the current task that
> > > wants
> > > to communicate with the daemon, it just wants to to access some
> > > directory on the system that just happens to be special to the
> > > kernel,
> > > which needs to do some communication on the side to service this
> > > request. So if we do want to do any LSM check here, there should
> > > at
> > > least be some "bool internal" flag passed to the LSM, signalizing
> > > that
> > > this is an internal read/write operation that wasn't directly
> > > initiated/requested by the current process. SELinux could then
> > > either
> > > use the kernel secid instead of the current task's secid or skip
> > > the
> > > check completely in such case.
> > 
> > Perhaps, but see below.
> > 
> > > I'd like Stephen to weigh in on this, but it looks he might be on
> > > vacation right now...
> > > 
> > > > > Any opinions?
> > > > 
> > > > Reexport for now.  Incidentally, what is LSM doing rejecting
> > > > writes
> > > > into a pipe?
> > > 
> > > With SELinux at least, what is allowed or denied is defined in
> > > the
> > > policy. And the policy usually defaults to everything denied and
> > > then
> > > you add rules to allow what needs (and makes sense) to be
> > > allowed.
> > > Since until kernel 5.8 random processes didn't need to write to
> > > pipes
> > > created by the automount daemon, it has never been explicitly
> > > allowed
> > > and so the automounting now fails. It is in no way obvious that
> > > all
> > > processes should have the permission to talk to the automount
> > > daemon
> > > just to traverse the filesystem...
> > 
> > I think you might have misunderstood what lead to this, just a bit.
> > 
> > Previously the __kern_write() function was used for this
> > communication
> > and Christoph's patch changed that to use kern_write() instead.
> > 
> > In theory that's a good idea because kern_write() adds some
> > additional
> > sanity checks, one being a call to rw_verify_area() which is where
> > the
> > security_file_permission() call fails.
> > 
> > So previously any random process could avoid these checks by
> > calling
> > __kern_write() so the change to kern_write() is, in theory, that's
> > a
> > good thing and simply reverting that hunk in Christoph's patch
> > probably isn't the best thing to do.
> 
> I understand that and I'm not proposing the revert as a long-term
> fix.
> For a long-term solution I propose using kernel_write() and extending
> it to allow the caller to suppress (just) the
> security_file_permission() call. Then each caller would have to
> decide
> whether the LSM check makes sense in that situation or not. It seems
> safer against future mistakes than leaving it up to the caller to
> call
> security_file_permission() explicitly (I predict that no new user
> would even realize that the call might be needed).
> 
> > But any random process does need to be able to write to the
> > automount
> > daemon pipe for trailing path components and the root dentry of
> > autofs
> > mounts, depending on case.
> > 
> > So it's true that any write to any autofs dentry probably doesn't
> > need to be allowed but I question what that gets us in terms of
> > security improvement over allowing pipe writes for automount_t
> > labelled pipes in selinux policy since they must be within an
> > autofs
> > mounted file system.
> 
> The difference is not in security, but in usability. The kernel
> communicating with the autofs daemon is an internal detail that
> shouldn't need special rules in policy. Even if we want to do any LSM
> checking here, the subject should be kernel_t, not the current
> running
> process. The process doesn't have any control on whether the kernel
> does the communication and it doesn't control the content of the
> communication, so the permission check as it is doesn't make any
> sense. People writing the policy should be burdened by low-level
> details about how the kernel works internally as little as possible.

I'm not "that" concerned about how it's done I'd just like to see
it fixed whether it's how I think it should be done or not.

> 
> > But Stephen has a different recommendation (and that appears to
> > consider the cause I outlined above) so I'll wait to see what
> > others
> > think about the recommendations.
> 
> As I said above, I think Stephen's approach is less future-proof. And
> it seems that rw_verify_area() has many other callers, most/all of
> which probably service actual requests from userspace and we'd need
> to
> retain the security_file_permission() call in those cases.
> 
> Let me try to put my proposal into a patch, so we have something
> concrete to talk about...

I think there was a suggestion of using function naming indicate
the different requirement, I think it was along the lines of
kern_write_nosec(), but if that approach is used the nosec is
misleading too, I'd prefer something like kern_write_internal()
to indicate clearly it's meant to cater for internal use.

But whatever, I'm keen to see what you recommend.

Ian

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

[Stephen Smalley]

On Thu, Sep 24, 2020 at 4:36 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:
>
> On Wed, Sep 23, 2020 at 3:55 AM Ian Kent <raven@themaw.net> wrote:
> > On Tue, 2020-09-22 at 09:33 +0200, Ondrej Mosnacek wrote:
> > > On Mon, Sep 21, 2020 at 6:30 PM Al Viro <viro@zeniv.linux.org.uk>
> > > wrote:
> > > > On Mon, Sep 21, 2020 at 06:09:22PM +0200, Christoph Hellwig wrote:
> > > > > [adding Linus and Al]
> > > > >
> > > > > On Mon, Sep 21, 2020 at 04:51:35PM +0200, Ondrej Mosnacek wrote:
> > > > > > Hi folks,
> > > > > >
> > > > > > It seems that after commit 13c164b1a186 ("autofs: switch to
> > > > > > kernel_write") there is now an extra LSM permission required
> > > > > > (for the
> > > > > > current task to write to the automount pipe) for processes
> > > > > > accessing
> > > > > > some yet-to-to-be mounted directory on which an autofs mount is
> > > > > > set
> > > > > > up. The call chain is:
> > > > > > [...]
> > > > > > autofs_wait() ->
> > > > > > autofs_notify_daemon() ->
> > > > > > autofs_write() ->
> > > > > > kernel_write() ->
> > > > > > rw_verify_area() ->
> > > > > > security_file_permission()
> > > > > >
> > > > > > The bug report that led me to this commit is at [1].
> > > > > >
> > > > > > Technically, this is a regression for LSM users, since this is
> > > > > > a
> > > > > > kernel-internal operation and an LSM permission for the current
> > > > > > task
> > > > > > shouldn't be required. Can this patch be reverted? Perhaps
> > > > > > __kernel_{read|write}() could instead be renamed to
> > > > > > kernel_*_nocheck()
> > > > > > so that the name is more descriptive?
> > > > >
> > > > > So we obviously should not break existing user space and need to
> > > > > fix
> > > > > this ASAP.  The trivial "fix" would be to export __kernel_write
> > > > > again
> > > > > and switch autofs to use it.  The other option would be a FMODE
> > > > > flag
> > > > > to bypass security checks, only to be set if the callers ensures
> > > > > they've been valided (i.e. in autofs_prepare_pipe).
> > >
> > > IMHO that sounds like an overkill in this scenario. I don't think it
> > > makes sense to do the LSM check here (or at least not against the
> > > current task's creds), because it is not the current task that wants
> > > to communicate with the daemon, it just wants to to access some
> > > directory on the system that just happens to be special to the
> > > kernel,
> > > which needs to do some communication on the side to service this
> > > request. So if we do want to do any LSM check here, there should at
> > > least be some "bool internal" flag passed to the LSM, signalizing
> > > that
> > > this is an internal read/write operation that wasn't directly
> > > initiated/requested by the current process. SELinux could then either
> > > use the kernel secid instead of the current task's secid or skip the
> > > check completely in such case.
> >
> > Perhaps, but see below.
> >
> > >
> > > I'd like Stephen to weigh in on this, but it looks he might be on
> > > vacation right now...
> > >
> > > > > Any opinions?
> > > >
> > > > Reexport for now.  Incidentally, what is LSM doing rejecting writes
> > > > into a pipe?
> > >
> > > With SELinux at least, what is allowed or denied is defined in the
> > > policy. And the policy usually defaults to everything denied and then
> > > you add rules to allow what needs (and makes sense) to be allowed.
> > > Since until kernel 5.8 random processes didn't need to write to pipes
> > > created by the automount daemon, it has never been explicitly allowed
> > > and so the automounting now fails. It is in no way obvious that all
> > > processes should have the permission to talk to the automount daemon
> > > just to traverse the filesystem...
> >
> > I think you might have misunderstood what lead to this, just a bit.
> >
> > Previously the __kern_write() function was used for this communication
> > and Christoph's patch changed that to use kern_write() instead.
> >
> > In theory that's a good idea because kern_write() adds some additional
> > sanity checks, one being a call to rw_verify_area() which is where the
> > security_file_permission() call fails.
> >
> > So previously any random process could avoid these checks by calling
> > __kern_write() so the change to kern_write() is, in theory, that's a
> > good thing and simply reverting that hunk in Christoph's patch
> > probably isn't the best thing to do.
>
> I understand that and I'm not proposing the revert as a long-term fix.
> For a long-term solution I propose using kernel_write() and extending
> it to allow the caller to suppress (just) the
> security_file_permission() call. Then each caller would have to decide
> whether the LSM check makes sense in that situation or not. It seems
> safer against future mistakes than leaving it up to the caller to call
> security_file_permission() explicitly (I predict that no new user
> would even realize that the call might be needed).
>
> >
> > But any random process does need to be able to write to the automount
> > daemon pipe for trailing path components and the root dentry of autofs
> > mounts, depending on case.
> >
> > So it's true that any write to any autofs dentry probably doesn't
> > need to be allowed but I question what that gets us in terms of
> > security improvement over allowing pipe writes for automount_t
> > labelled pipes in selinux policy since they must be within an autofs
> > mounted file system.
>
> The difference is not in security, but in usability. The kernel
> communicating with the autofs daemon is an internal detail that
> shouldn't need special rules in policy. Even if we want to do any LSM
> checking here, the subject should be kernel_t, not the current running
> process. The process doesn't have any control on whether the kernel
> does the communication and it doesn't control the content of the
> communication, so the permission check as it is doesn't make any
> sense. People writing the policy should be burdened by low-level
> details about how the kernel works internally as little as possible.
>
> >
> > But Stephen has a different recommendation (and that appears to
> > consider the cause I outlined above) so I'll wait to see what others
> > think about the recommendations.
>
> As I said above, I think Stephen's approach is less future-proof. And
> it seems that rw_verify_area() has many other callers, most/all of
> which probably service actual requests from userspace and we'd need to
> retain the security_file_permission() call in those cases.
>
> Let me try to put my proposal into a patch, so we have something
> concrete to talk about...

Up-thread I thought Linus indicated he didn't really want a flag to
disable pemission checking due to potential abuse (and I agree).
Historically we have taken one of two approaches for these situations:
1) Provide a separate interface like kernel_write() for use when we
don't want permission checking and don't have it call the security
hook at all.  If you prefer kernel_write_nosec() that's fine but I
think that's somewhat implicit in the fact that it is a
kernel-initiated write, not a userspace write (which would hopefully
go through vfs_write() or similar and end up calling the hook).
2) Temporarily override creds to the init_cred or the result of
prepare_kernel_creds() before calling any credential-checking
functions and then revert creds afterward.

The problem with #2 is that it still requires that the policy allow
kernel_t (or its equivalent) to be able to write to these pipes, which
wasn't previously necessary and thus might not be allowed in all
policies (e.g. Android?).  #1 avoids any need for policy for these
operations.

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

[Ian Kent]

On Thu, 2020-09-24 at 10:16 -0400, Stephen Smalley wrote:
> On Thu, Sep 24, 2020 at 4:36 AM Ondrej Mosnacek <omosnace@redhat.com>
> wrote:
> > On Wed, Sep 23, 2020 at 3:55 AM Ian Kent <raven@themaw.net> wrote:
> > > On Tue, 2020-09-22 at 09:33 +0200, Ondrej Mosnacek wrote:
> > > > On Mon, Sep 21, 2020 at 6:30 PM Al Viro <
> > > > viro@zeniv.linux.org.uk>
> > > > wrote:
> > > > > On Mon, Sep 21, 2020 at 06:09:22PM +0200, Christoph Hellwig
> > > > > wrote:
> > > > > > [adding Linus and Al]
> > > > > > 
> > > > > > On Mon, Sep 21, 2020 at 04:51:35PM +0200, Ondrej Mosnacek
> > > > > > wrote:
> > > > > > > Hi folks,
> > > > > > > 
> > > > > > > It seems that after commit 13c164b1a186 ("autofs: switch
> > > > > > > to
> > > > > > > kernel_write") there is now an extra LSM permission
> > > > > > > required
> > > > > > > (for the
> > > > > > > current task to write to the automount pipe) for
> > > > > > > processes
> > > > > > > accessing
> > > > > > > some yet-to-to-be mounted directory on which an autofs
> > > > > > > mount is
> > > > > > > set
> > > > > > > up. The call chain is:
> > > > > > > [...]
> > > > > > > autofs_wait() ->
> > > > > > > autofs_notify_daemon() ->
> > > > > > > autofs_write() ->
> > > > > > > kernel_write() ->
> > > > > > > rw_verify_area() ->
> > > > > > > security_file_permission()
> > > > > > > 
> > > > > > > The bug report that led me to this commit is at [1].
> > > > > > > 
> > > > > > > Technically, this is a regression for LSM users, since
> > > > > > > this is
> > > > > > > a
> > > > > > > kernel-internal operation and an LSM permission for the
> > > > > > > current
> > > > > > > task
> > > > > > > shouldn't be required. Can this patch be reverted?
> > > > > > > Perhaps
> > > > > > > __kernel_{read|write}() could instead be renamed to
> > > > > > > kernel_*_nocheck()
> > > > > > > so that the name is more descriptive?
> > > > > > 
> > > > > > So we obviously should not break existing user space and
> > > > > > need to
> > > > > > fix
> > > > > > this ASAP.  The trivial "fix" would be to export
> > > > > > __kernel_write
> > > > > > again
> > > > > > and switch autofs to use it.  The other option would be a
> > > > > > FMODE
> > > > > > flag
> > > > > > to bypass security checks, only to be set if the callers
> > > > > > ensures
> > > > > > they've been valided (i.e. in autofs_prepare_pipe).
> > > > 
> > > > IMHO that sounds like an overkill in this scenario. I don't
> > > > think it
> > > > makes sense to do the LSM check here (or at least not against
> > > > the
> > > > current task's creds), because it is not the current task that
> > > > wants
> > > > to communicate with the daemon, it just wants to to access some
> > > > directory on the system that just happens to be special to the
> > > > kernel,
> > > > which needs to do some communication on the side to service
> > > > this
> > > > request. So if we do want to do any LSM check here, there
> > > > should at
> > > > least be some "bool internal" flag passed to the LSM,
> > > > signalizing
> > > > that
> > > > this is an internal read/write operation that wasn't directly
> > > > initiated/requested by the current process. SELinux could then
> > > > either
> > > > use the kernel secid instead of the current task's secid or
> > > > skip the
> > > > check completely in such case.
> > > 
> > > Perhaps, but see below.
> > > 
> > > > I'd like Stephen to weigh in on this, but it looks he might be
> > > > on
> > > > vacation right now...
> > > > 
> > > > > > Any opinions?
> > > > > 
> > > > > Reexport for now.  Incidentally, what is LSM doing rejecting
> > > > > writes
> > > > > into a pipe?
> > > > 
> > > > With SELinux at least, what is allowed or denied is defined in
> > > > the
> > > > policy. And the policy usually defaults to everything denied
> > > > and then
> > > > you add rules to allow what needs (and makes sense) to be
> > > > allowed.
> > > > Since until kernel 5.8 random processes didn't need to write to
> > > > pipes
> > > > created by the automount daemon, it has never been explicitly
> > > > allowed
> > > > and so the automounting now fails. It is in no way obvious that
> > > > all
> > > > processes should have the permission to talk to the automount
> > > > daemon
> > > > just to traverse the filesystem...
> > > 
> > > I think you might have misunderstood what lead to this, just a
> > > bit.
> > > 
> > > Previously the __kern_write() function was used for this
> > > communication
> > > and Christoph's patch changed that to use kern_write() instead.
> > > 
> > > In theory that's a good idea because kern_write() adds some
> > > additional
> > > sanity checks, one being a call to rw_verify_area() which is
> > > where the
> > > security_file_permission() call fails.
> > > 
> > > So previously any random process could avoid these checks by
> > > calling
> > > __kern_write() so the change to kern_write() is, in theory,
> > > that's a
> > > good thing and simply reverting that hunk in Christoph's patch
> > > probably isn't the best thing to do.
> > 
> > I understand that and I'm not proposing the revert as a long-term
> > fix.
> > For a long-term solution I propose using kernel_write() and
> > extending
> > it to allow the caller to suppress (just) the
> > security_file_permission() call. Then each caller would have to
> > decide
> > whether the LSM check makes sense in that situation or not. It
> > seems
> > safer against future mistakes than leaving it up to the caller to
> > call
> > security_file_permission() explicitly (I predict that no new user
> > would even realize that the call might be needed).
> > 
> > > But any random process does need to be able to write to the
> > > automount
> > > daemon pipe for trailing path components and the root dentry of
> > > autofs
> > > mounts, depending on case.
> > > 
> > > So it's true that any write to any autofs dentry probably doesn't
> > > need to be allowed but I question what that gets us in terms of
> > > security improvement over allowing pipe writes for automount_t
> > > labelled pipes in selinux policy since they must be within an
> > > autofs
> > > mounted file system.
> > 
> > The difference is not in security, but in usability. The kernel
> > communicating with the autofs daemon is an internal detail that
> > shouldn't need special rules in policy. Even if we want to do any
> > LSM
> > checking here, the subject should be kernel_t, not the current
> > running
> > process. The process doesn't have any control on whether the kernel
> > does the communication and it doesn't control the content of the
> > communication, so the permission check as it is doesn't make any
> > sense. People writing the policy should be burdened by low-level
> > details about how the kernel works internally as little as
> > possible.
> > 
> > > But Stephen has a different recommendation (and that appears to
> > > consider the cause I outlined above) so I'll wait to see what
> > > others
> > > think about the recommendations.
> > 
> > As I said above, I think Stephen's approach is less future-proof.
> > And
> > it seems that rw_verify_area() has many other callers, most/all of
> > which probably service actual requests from userspace and we'd need
> > to
> > retain the security_file_permission() call in those cases.
> > 
> > Let me try to put my proposal into a patch, so we have something
> > concrete to talk about...
> 
> Up-thread I thought Linus indicated he didn't really want a flag to
> disable pemission checking due to potential abuse (and I agree).
> Historically we have taken one of two approaches for these
> situations:
> 1) Provide a separate interface like kernel_write() for use when we
> don't want permission checking and don't have it call the security
> hook at all.  If you prefer kernel_write_nosec() that's fine but I
> think that's somewhat implicit in the fact that it is a
> kernel-initiated write, not a userspace write (which would hopefully
> go through vfs_write() or similar and end up calling the hook).
> 2) Temporarily override creds to the init_cred or the result of
> prepare_kernel_creds() before calling any credential-checking
> functions and then revert creds afterward.
> 
> The problem with #2 is that it still requires that the policy allow
> kernel_t (or its equivalent) to be able to write to these pipes,
> which
> wasn't previously necessary and thus might not be allowed in all
> policies (e.g. Android?).  #1 avoids any need for policy for these
> operations.

There is a third option, that's actually to revert only the autofs
change and forgo the couple of extra checks we get from kern_write().

My recommendation of keeping the change and documenting the behaviour
via security policy includes the implicit assumption that there are
no other cases of this.

If that wasn't the case then changing the kernel functions or (likely
better option of) simply using __kern_write() in those cases would be
the better choice IMHO.

Ian

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

[Ian Kent]

On Fri, 2020-09-25 at 11:37 +0800, Ian Kent wrote:
> On Thu, 2020-09-24 at 10:16 -0400, Stephen Smalley wrote:
> > On Thu, Sep 24, 2020 at 4:36 AM Ondrej Mosnacek <
> > omosnace@redhat.com>
> > wrote:
> > > On Wed, Sep 23, 2020 at 3:55 AM Ian Kent <raven@themaw.net>
> > > wrote:
> > > > On Tue, 2020-09-22 at 09:33 +0200, Ondrej Mosnacek wrote:
> > > > > On Mon, Sep 21, 2020 at 6:30 PM Al Viro <
> > > > > viro@zeniv.linux.org.uk>
> > > > > wrote:
> > > > > > On Mon, Sep 21, 2020 at 06:09:22PM +0200, Christoph Hellwig
> > > > > > wrote:
> > > > > > > [adding Linus and Al]
> > > > > > > 
> > > > > > > On Mon, Sep 21, 2020 at 04:51:35PM +0200, Ondrej Mosnacek
> > > > > > > wrote:
> > > > > > > > Hi folks,
> > > > > > > > 
> > > > > > > > It seems that after commit 13c164b1a186 ("autofs:
> > > > > > > > switch
> > > > > > > > to
> > > > > > > > kernel_write") there is now an extra LSM permission
> > > > > > > > required
> > > > > > > > (for the
> > > > > > > > current task to write to the automount pipe) for
> > > > > > > > processes
> > > > > > > > accessing
> > > > > > > > some yet-to-to-be mounted directory on which an autofs
> > > > > > > > mount is
> > > > > > > > set
> > > > > > > > up. The call chain is:
> > > > > > > > [...]
> > > > > > > > autofs_wait() ->
> > > > > > > > autofs_notify_daemon() ->
> > > > > > > > autofs_write() ->
> > > > > > > > kernel_write() ->
> > > > > > > > rw_verify_area() ->
> > > > > > > > security_file_permission()
> > > > > > > > 
> > > > > > > > The bug report that led me to this commit is at [1].
> > > > > > > > 
> > > > > > > > Technically, this is a regression for LSM users, since
> > > > > > > > this is
> > > > > > > > a
> > > > > > > > kernel-internal operation and an LSM permission for the
> > > > > > > > current
> > > > > > > > task
> > > > > > > > shouldn't be required. Can this patch be reverted?
> > > > > > > > Perhaps
> > > > > > > > __kernel_{read|write}() could instead be renamed to
> > > > > > > > kernel_*_nocheck()
> > > > > > > > so that the name is more descriptive?
> > > > > > > 
> > > > > > > So we obviously should not break existing user space and
> > > > > > > need to
> > > > > > > fix
> > > > > > > this ASAP.  The trivial "fix" would be to export
> > > > > > > __kernel_write
> > > > > > > again
> > > > > > > and switch autofs to use it.  The other option would be a
> > > > > > > FMODE
> > > > > > > flag
> > > > > > > to bypass security checks, only to be set if the callers
> > > > > > > ensures
> > > > > > > they've been valided (i.e. in autofs_prepare_pipe).
> > > > > 
> > > > > IMHO that sounds like an overkill in this scenario. I don't
> > > > > think it
> > > > > makes sense to do the LSM check here (or at least not against
> > > > > the
> > > > > current task's creds), because it is not the current task
> > > > > that
> > > > > wants
> > > > > to communicate with the daemon, it just wants to to access
> > > > > some
> > > > > directory on the system that just happens to be special to
> > > > > the
> > > > > kernel,
> > > > > which needs to do some communication on the side to service
> > > > > this
> > > > > request. So if we do want to do any LSM check here, there
> > > > > should at
> > > > > least be some "bool internal" flag passed to the LSM,
> > > > > signalizing
> > > > > that
> > > > > this is an internal read/write operation that wasn't directly
> > > > > initiated/requested by the current process. SELinux could
> > > > > then
> > > > > either
> > > > > use the kernel secid instead of the current task's secid or
> > > > > skip the
> > > > > check completely in such case.
> > > > 
> > > > Perhaps, but see below.
> > > > 
> > > > > I'd like Stephen to weigh in on this, but it looks he might
> > > > > be
> > > > > on
> > > > > vacation right now...
> > > > > 
> > > > > > > Any opinions?
> > > > > > 
> > > > > > Reexport for now.  Incidentally, what is LSM doing
> > > > > > rejecting
> > > > > > writes
> > > > > > into a pipe?
> > > > > 
> > > > > With SELinux at least, what is allowed or denied is defined
> > > > > in
> > > > > the
> > > > > policy. And the policy usually defaults to everything denied
> > > > > and then
> > > > > you add rules to allow what needs (and makes sense) to be
> > > > > allowed.
> > > > > Since until kernel 5.8 random processes didn't need to write
> > > > > to
> > > > > pipes
> > > > > created by the automount daemon, it has never been explicitly
> > > > > allowed
> > > > > and so the automounting now fails. It is in no way obvious
> > > > > that
> > > > > all
> > > > > processes should have the permission to talk to the automount
> > > > > daemon
> > > > > just to traverse the filesystem...
> > > > 
> > > > I think you might have misunderstood what lead to this, just a
> > > > bit.
> > > > 
> > > > Previously the __kern_write() function was used for this
> > > > communication
> > > > and Christoph's patch changed that to use kern_write() instead.
> > > > 
> > > > In theory that's a good idea because kern_write() adds some
> > > > additional
> > > > sanity checks, one being a call to rw_verify_area() which is
> > > > where the
> > > > security_file_permission() call fails.
> > > > 
> > > > So previously any random process could avoid these checks by
> > > > calling
> > > > __kern_write() so the change to kern_write() is, in theory,
> > > > that's a
> > > > good thing and simply reverting that hunk in Christoph's patch
> > > > probably isn't the best thing to do.
> > > 
> > > I understand that and I'm not proposing the revert as a long-term
> > > fix.
> > > For a long-term solution I propose using kernel_write() and
> > > extending
> > > it to allow the caller to suppress (just) the
> > > security_file_permission() call. Then each caller would have to
> > > decide
> > > whether the LSM check makes sense in that situation or not. It
> > > seems
> > > safer against future mistakes than leaving it up to the caller to
> > > call
> > > security_file_permission() explicitly (I predict that no new user
> > > would even realize that the call might be needed).
> > > 
> > > > But any random process does need to be able to write to the
> > > > automount
> > > > daemon pipe for trailing path components and the root dentry of
> > > > autofs
> > > > mounts, depending on case.
> > > > 
> > > > So it's true that any write to any autofs dentry probably
> > > > doesn't
> > > > need to be allowed but I question what that gets us in terms of
> > > > security improvement over allowing pipe writes for automount_t
> > > > labelled pipes in selinux policy since they must be within an
> > > > autofs
> > > > mounted file system.
> > > 
> > > The difference is not in security, but in usability. The kernel
> > > communicating with the autofs daemon is an internal detail that
> > > shouldn't need special rules in policy. Even if we want to do any
> > > LSM
> > > checking here, the subject should be kernel_t, not the current
> > > running
> > > process. The process doesn't have any control on whether the
> > > kernel
> > > does the communication and it doesn't control the content of the
> > > communication, so the permission check as it is doesn't make any
> > > sense. People writing the policy should be burdened by low-level
> > > details about how the kernel works internally as little as
> > > possible.
> > > 
> > > > But Stephen has a different recommendation (and that appears to
> > > > consider the cause I outlined above) so I'll wait to see what
> > > > others
> > > > think about the recommendations.
> > > 
> > > As I said above, I think Stephen's approach is less future-proof.
> > > And
> > > it seems that rw_verify_area() has many other callers, most/all
> > > of
> > > which probably service actual requests from userspace and we'd
> > > need
> > > to
> > > retain the security_file_permission() call in those cases.
> > > 
> > > Let me try to put my proposal into a patch, so we have something
> > > concrete to talk about...
> > 
> > Up-thread I thought Linus indicated he didn't really want a flag to
> > disable pemission checking due to potential abuse (and I agree).
> > Historically we have taken one of two approaches for these
> > situations:
> > 1) Provide a separate interface like kernel_write() for use when we
> > don't want permission checking and don't have it call the security
> > hook at all.  If you prefer kernel_write_nosec() that's fine but I
> > think that's somewhat implicit in the fact that it is a
> > kernel-initiated write, not a userspace write (which would
> > hopefully
> > go through vfs_write() or similar and end up calling the hook).
> > 2) Temporarily override creds to the init_cred or the result of
> > prepare_kernel_creds() before calling any credential-checking
> > functions and then revert creds afterward.
> > 
> > The problem with #2 is that it still requires that the policy allow
> > kernel_t (or its equivalent) to be able to write to these pipes,
> > which
> > wasn't previously necessary and thus might not be allowed in all
> > policies (e.g. Android?).  #1 avoids any need for policy for these
> > operations.
> 
> There is a third option, that's actually to revert only the autofs
> change and forgo the couple of extra checks we get from kern_write().
> 
> My recommendation of keeping the change and documenting the behaviour
> via security policy includes the implicit assumption that there are
> no other cases of this.
> 
> If that wasn't the case then changing the kernel functions or (likely
> better option of) simply using __kern_write() in those cases would be
> the better choice IMHO.

Oh wait, that is essentially option one after re-reading Stephens
comment again ...

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

[Ondrej Mosnacek]

On Thu, Sep 24, 2020 at 4:16 PM Stephen Smalley
<stephen.smalley.work@gmail.com> wrote:
> On Thu, Sep 24, 2020 at 4:36 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:
> >
> > On Wed, Sep 23, 2020 at 3:55 AM Ian Kent <raven@themaw.net> wrote:
> > > On Tue, 2020-09-22 at 09:33 +0200, Ondrej Mosnacek wrote:
> > > > On Mon, Sep 21, 2020 at 6:30 PM Al Viro <viro@zeniv.linux.org.uk>
> > > > wrote:
> > > > > On Mon, Sep 21, 2020 at 06:09:22PM +0200, Christoph Hellwig wrote:
> > > > > > [adding Linus and Al]
> > > > > >
> > > > > > On Mon, Sep 21, 2020 at 04:51:35PM +0200, Ondrej Mosnacek wrote:
> > > > > > > Hi folks,
> > > > > > >
> > > > > > > It seems that after commit 13c164b1a186 ("autofs: switch to
> > > > > > > kernel_write") there is now an extra LSM permission required
> > > > > > > (for the
> > > > > > > current task to write to the automount pipe) for processes
> > > > > > > accessing
> > > > > > > some yet-to-to-be mounted directory on which an autofs mount is
> > > > > > > set
> > > > > > > up. The call chain is:
> > > > > > > [...]
> > > > > > > autofs_wait() ->
> > > > > > > autofs_notify_daemon() ->
> > > > > > > autofs_write() ->
> > > > > > > kernel_write() ->
> > > > > > > rw_verify_area() ->
> > > > > > > security_file_permission()
> > > > > > >
> > > > > > > The bug report that led me to this commit is at [1].
> > > > > > >
> > > > > > > Technically, this is a regression for LSM users, since this is
> > > > > > > a
> > > > > > > kernel-internal operation and an LSM permission for the current
> > > > > > > task
> > > > > > > shouldn't be required. Can this patch be reverted? Perhaps
> > > > > > > __kernel_{read|write}() could instead be renamed to
> > > > > > > kernel_*_nocheck()
> > > > > > > so that the name is more descriptive?
> > > > > >
> > > > > > So we obviously should not break existing user space and need to
> > > > > > fix
> > > > > > this ASAP.  The trivial "fix" would be to export __kernel_write
> > > > > > again
> > > > > > and switch autofs to use it.  The other option would be a FMODE
> > > > > > flag
> > > > > > to bypass security checks, only to be set if the callers ensures
> > > > > > they've been valided (i.e. in autofs_prepare_pipe).
> > > >
> > > > IMHO that sounds like an overkill in this scenario. I don't think it
> > > > makes sense to do the LSM check here (or at least not against the
> > > > current task's creds), because it is not the current task that wants
> > > > to communicate with the daemon, it just wants to to access some
> > > > directory on the system that just happens to be special to the
> > > > kernel,
> > > > which needs to do some communication on the side to service this
> > > > request. So if we do want to do any LSM check here, there should at
> > > > least be some "bool internal" flag passed to the LSM, signalizing
> > > > that
> > > > this is an internal read/write operation that wasn't directly
> > > > initiated/requested by the current process. SELinux could then either
> > > > use the kernel secid instead of the current task's secid or skip the
> > > > check completely in such case.
> > >
> > > Perhaps, but see below.
> > >
> > > >
> > > > I'd like Stephen to weigh in on this, but it looks he might be on
> > > > vacation right now...
> > > >
> > > > > > Any opinions?
> > > > >
> > > > > Reexport for now.  Incidentally, what is LSM doing rejecting writes
> > > > > into a pipe?
> > > >
> > > > With SELinux at least, what is allowed or denied is defined in the
> > > > policy. And the policy usually defaults to everything denied and then
> > > > you add rules to allow what needs (and makes sense) to be allowed.
> > > > Since until kernel 5.8 random processes didn't need to write to pipes
> > > > created by the automount daemon, it has never been explicitly allowed
> > > > and so the automounting now fails. It is in no way obvious that all
> > > > processes should have the permission to talk to the automount daemon
> > > > just to traverse the filesystem...
> > >
> > > I think you might have misunderstood what lead to this, just a bit.
> > >
> > > Previously the __kern_write() function was used for this communication
> > > and Christoph's patch changed that to use kern_write() instead.
> > >
> > > In theory that's a good idea because kern_write() adds some additional
> > > sanity checks, one being a call to rw_verify_area() which is where the
> > > security_file_permission() call fails.
> > >
> > > So previously any random process could avoid these checks by calling
> > > __kern_write() so the change to kern_write() is, in theory, that's a
> > > good thing and simply reverting that hunk in Christoph's patch
> > > probably isn't the best thing to do.
> >
> > I understand that and I'm not proposing the revert as a long-term fix.
> > For a long-term solution I propose using kernel_write() and extending
> > it to allow the caller to suppress (just) the
> > security_file_permission() call. Then each caller would have to decide
> > whether the LSM check makes sense in that situation or not. It seems
> > safer against future mistakes than leaving it up to the caller to call
> > security_file_permission() explicitly (I predict that no new user
> > would even realize that the call might be needed).
> >
> > >
> > > But any random process does need to be able to write to the automount
> > > daemon pipe for trailing path components and the root dentry of autofs
> > > mounts, depending on case.
> > >
> > > So it's true that any write to any autofs dentry probably doesn't
> > > need to be allowed but I question what that gets us in terms of
> > > security improvement over allowing pipe writes for automount_t
> > > labelled pipes in selinux policy since they must be within an autofs
> > > mounted file system.
> >
> > The difference is not in security, but in usability. The kernel
> > communicating with the autofs daemon is an internal detail that
> > shouldn't need special rules in policy. Even if we want to do any LSM
> > checking here, the subject should be kernel_t, not the current running
> > process. The process doesn't have any control on whether the kernel
> > does the communication and it doesn't control the content of the
> > communication, so the permission check as it is doesn't make any
> > sense. People writing the policy should be burdened by low-level
> > details about how the kernel works internally as little as possible.
> >
> > >
> > > But Stephen has a different recommendation (and that appears to
> > > consider the cause I outlined above) so I'll wait to see what others
> > > think about the recommendations.
> >
> > As I said above, I think Stephen's approach is less future-proof. And
> > it seems that rw_verify_area() has many other callers, most/all of
> > which probably service actual requests from userspace and we'd need to
> > retain the security_file_permission() call in those cases.
> >
> > Let me try to put my proposal into a patch, so we have something
> > concrete to talk about...
>
> 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). The latter is less prone to accidents and
any new user would have to set that parameter to *something* and so
would be forced to think about whether he needs an LSM check or not.
Whereas with a separate _nosec()/nolsmcheck()/_whatever() function
they would likely just use the kernel_write() function they already
know and may not even realize that there is some decision to be made.

But that's just my opinion, I would still be happier with a separate
function than reverting to __kernel_write().

> Historically we have taken one of two approaches for these situations:
> 1) Provide a separate interface like kernel_write() for use when we
> don't want permission checking and don't have it call the security
> hook at all.  If you prefer kernel_write_nosec() that's fine but I
> think that's somewhat implicit in the fact that it is a
> kernel-initiated write, not a userspace write (which would hopefully
> go through vfs_write() or similar and end up calling the hook).

Unfortunately not all of the kernel_write() calls seem to be of the
kind that we want to skip the permission checking. One in particular
that caught my attention is this btrfs one:
https://elixir.bootlin.com/linux/v5.9-rc6/source/fs/btrfs/send.c#L561

Basically a userspace process passes some fd via ioctl and kernel then
writes something in it. If we don't do any check here (and BTW we in
fact want the current's creds to apply here), then the process could
pass any open fd and bypass any LSM write permissions to the file
behind that fd. Possibly there are some checks that restrict what can
be passed there (I didn't check), but it would still be fragile to
rely on them.

So I guess we can't just skip the check whenever the kernel_*()
function is called.

> 2) Temporarily override creds to the init_cred or the result of
> prepare_kernel_creds() before calling any credential-checking
> functions and then revert creds afterward.

Most users of kernel_write() seem to also create/open the file
themselves, usually using the current's creds, so this would break
consistency when applied universally. Autofs (and also
bpfilter_send_req()) are special in that they first get the fd from a
userspace process and then write into it from other tasks' syscalls.
Then there are one or two users that create the file using
shmem_kernel_file_setup(), which does use prepare_kernel_creds(), but
the resulting file is marked S_PRIVATE, so all LSM checks are already
skipped for it anyway.

>
> The problem with #2 is that it still requires that the policy allow
> kernel_t (or its equivalent) to be able to write to these pipes, which
> wasn't previously necessary and thus might not be allowed in all
> policies (e.g. Android?).  #1 avoids any need for policy for these
> operations.

Yeah, I'm starting to think checking "kernel creds -> userspace
object" access vectors on purpose doesn't make much sense... you would
pretty much want to always allow that (no point in kernel blocking its
own operations) so just skipping the check in those cases where using
the current's creds doesn't make sense seems to be a better choice.

BTW, I don't have the candidate patch written yet, as I haven't had
time and also need to process new information after having reviewed
the existing callers.

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

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

[Linus Torvalds]

On Fri, Sep 25, 2020 at 6:38 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:
>
> On Thu, Sep 24, 2020 at 4:16 PM Stephen Smalley
> <stephen.smalley.work@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?

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").

Hmm?

             Linus

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

[Ian Kent]

On Fri, 2020-09-25 at 10:38 -0700, Linus Torvalds wrote:
> On Fri, Sep 25, 2020 at 6:38 AM Ondrej Mosnacek <omosnace@redhat.com>
> wrote:
> > On Thu, Sep 24, 2020 at 4:16 PM Stephen Smalley
> > <stephen.smalley.work@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. 

Ian

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

[Ondrej Mosnacek]

On Sun, Sep 27, 2020 at 5:08 AM Ian Kent <raven@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@redhat.com>
> > wrote:
> > > On Thu, Sep 24, 2020 at 4:16 PM Stephen Smalley
> > > <stephen.smalley.work@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.

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

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

[Linus Torvalds]

[-- Attachment #1: Type: text/plain, Size: 785 bytes --]

On Tue, Sep 29, 2020 at 5:16 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:
>
> OK, so it seems that reverting comes out as the best choice here.

Yeah.

> 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)

Ok, that doesn't matter for the security_file_permission() issue, but
yes, autofs is doing the traditional thing, and it's pointless. Using
file_ppos(file) isn't an option since it's an inline to read_write.c,
but it could just pass in NULL these days and avoid that too.

So how about we just do the appended patch? Can whoever sees this
problem just verify, even though it looks trivially correct...

            Linus

[-- Attachment #2: patch --]
[-- Type: application/octet-stream, Size: 909 bytes --]

 fs/autofs/waitq.c | 2 +-
 fs/read_write.c   | 1 +
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/fs/autofs/waitq.c b/fs/autofs/waitq.c
index 74c886f7c51c..5ced859dac53 100644
--- a/fs/autofs/waitq.c
+++ b/fs/autofs/waitq.c
@@ -53,7 +53,7 @@ static int autofs_write(struct autofs_sb_info *sbi,
 
 	mutex_lock(&sbi->pipe_mutex);
 	while (bytes) {
-		wr = kernel_write(file, data, bytes, &file->f_pos);
+		wr = __kernel_write(file, data, bytes, NULL);
 		if (wr <= 0)
 			break;
 		data += wr;
diff --git a/fs/read_write.c b/fs/read_write.c
index 5db58b8c78d0..2fc3194e4d30 100644
--- a/fs/read_write.c
+++ b/fs/read_write.c
@@ -538,6 +538,7 @@ ssize_t __kernel_write(struct file *file, const void *buf, size_t count, loff_t
 	inc_syscw(current);
 	return ret;
 }
+EXPORT_SYMBOL_GPL(__kernel_write);
 
 ssize_t kernel_write(struct file *file, const void *buf, size_t count,
 			    loff_t *pos)

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

[Christoph Hellwig]

On Tue, Sep 29, 2020 at 10:23:50AM -0700, Linus Torvalds wrote:
> On Tue, Sep 29, 2020 at 5:16 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:
> >
> > OK, so it seems that reverting comes out as the best choice here.
> 
> Yeah.
> 
> > 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)
> 
> Ok, that doesn't matter for the security_file_permission() issue, but
> yes, autofs is doing the traditional thing, and it's pointless. Using
> file_ppos(file) isn't an option since it's an inline to read_write.c,
> but it could just pass in NULL these days and avoid that too.
> 
> So how about we just do the appended patch? Can whoever sees this
> problem just verify, even though it looks trivially correct...

This looks sensible to me.  I'd throw in a

/* only for autofs, don't use in new code */

near the export, but users of these kind of functions tend to be
blind copy and paste code anyway, so the comment probably isn't
even read by the relevant parties..

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

[Ian Kent]

On Tue, 2020-09-29 at 20:00 +0200, Christoph Hellwig wrote:
> On Tue, Sep 29, 2020 at 10:23:50AM -0700, Linus Torvalds wrote:
> > On Tue, Sep 29, 2020 at 5:16 AM Ondrej Mosnacek <
> > omosnace@redhat.com> wrote:
> > > OK, so it seems that reverting comes out as the best choice here.
> > 
> > Yeah.
> > 
> > > 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)
> > 
> > Ok, that doesn't matter for the security_file_permission() issue,
> > but
> > yes, autofs is doing the traditional thing, and it's pointless.
> > Using
> > file_ppos(file) isn't an option since it's an inline to
> > read_write.c,
> > but it could just pass in NULL these days and avoid that too.
> > 
> > So how about we just do the appended patch? Can whoever sees this
> > problem just verify, even though it looks trivially correct...
> 
> This looks sensible to me.  I'd throw in a
> 
> /* only for autofs, don't use in new code */
> 
> near the export, but users of these kind of functions tend to be
> blind copy and paste code anyway, so the comment probably isn't
> even read by the relevant parties..

I'll build a patched kernel and give it a whirl.

Ian

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

[Ian Kent]

On Wed, 2020-09-30 at 13:50 +0800, Ian Kent wrote:
> On Tue, 2020-09-29 at 20:00 +0200, Christoph Hellwig wrote:
> > On Tue, Sep 29, 2020 at 10:23:50AM -0700, Linus Torvalds wrote:
> > > On Tue, Sep 29, 2020 at 5:16 AM Ondrej Mosnacek <
> > > omosnace@redhat.com> wrote:
> > > > OK, so it seems that reverting comes out as the best choice
> > > > here.
> > > 
> > > Yeah.
> > > 
> > > > 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)
> > > 
> > > Ok, that doesn't matter for the security_file_permission() issue,
> > > but
> > > yes, autofs is doing the traditional thing, and it's pointless.
> > > Using
> > > file_ppos(file) isn't an option since it's an inline to
> > > read_write.c,
> > > but it could just pass in NULL these days and avoid that too.
> > > 
> > > So how about we just do the appended patch? Can whoever sees this
> > > problem just verify, even though it looks trivially correct...
> > 
> > This looks sensible to me.  I'd throw in a
> > 
> > /* only for autofs, don't use in new code */
> > 
> > near the export, but users of these kind of functions tend to be
> > blind copy and paste code anyway, so the comment probably isn't
> > even read by the relevant parties..
> 
> I'll build a patched kernel and give it a whirl.

Duplicated the problem first then built a patched kernel and tested.

The patch makes the problem go away, communication to the daemon
is occurring fine so the use of NULL as the file position is ok.

It looks like the error returns should be as before which is good
but there may be another problem I need to fix in there that became
apparent because this problem drew attention to it. That's something
else entirely though.

> 
> Ian

Re: 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@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@redhat.com>
> > > wrote:
> > > > On Thu, Sep 24, 2020 at 4:16 PM Stephen Smalley
> > > > <stephen.smalley.work@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