Question about security_file_open contract

[Martin Kelly]

> -----Original Message-----
> From: Casey Schaufler <casey at schaufler-ca.com>
> Sent: Wednesday, November 18, 2020 1:26 PM
> To: Martin Kelly <martin.kelly at crowdstrike.com>; linux-security-
> module at vger.kernel.org
> Cc: Casey Schaufler <casey at schaufler-ca.com>
> Subject: [External] Re: Question about security_file_open contract
>
> On 11/18/2020 12:44 PM, Martin Kelly wrote:
> > - A process exits, calling task_exit().
> > - exit_fs() is called, setting current->fs = NULL.
> > - Next, exit_task_work() is called, which calls fput().
> > - In response to the fput(), the filesystem opens a file to update
> > some metadata, calling dentry_open().
>
> Which process do you expect the file to be opened for? As you point out,
> current is being torn down. What is your kernel driver doing that led you to
> think this was a good idea?
>
>

The driver is the OpenAFS filesystem. I'm not a developer for or expert in 
OpenAFS, but from the stack, it appears to be updating a disk transaction 
database to reflect a removed directory. It seems this happens to work most of 
the time but does not play nice with LSMs.

> > - dentry_open() calls security_file_open(), calling into the LSM. The
> > LSM crashes because it assumes it's called from process context and
> > thus
> > current->fs is not NULL.
>
> Right. Without a valid process context it's impossible to make access 
> control
> decisions.
>
> >
> > I'm trying to figure out exactly what the contract is here. Is it safe
> > for an LSM to assume current->fs should be non-NULL when
> > security_file_open is called?  More generally, is it safe for an LSM
> > to assume that security_file_open will always be called from process
> > context? In other words, is the LSM at fault here or the driver?
>
> The driver. If you want to open files directly from the driver, as opposed 
> to
> from a process, you need a valid kernel context.
>

OK, this is what I figured; thanks for confirming. Sounds like OpenAFS should 
be patched to do this work in a kernel context.

> > Thanks,
> > Martin