Re: [RFC][PATCH] net/bpfilter: Remove this broken and apparently unmantained

[Alexei Starovoitov <alexei.starovoitov@gmail.com>]

On Wed, Jun 24, 2020 at 10:51:15AM +0900, Tetsuo Handa wrote:
> On 2020/06/24 4:40, Alexei Starovoitov wrote:
> > There is no refcnt bug. It was a user error on tomoyo side.
> > fork_blob() works as expected.
> 
> Absolutely wrong! Any check which returns an error during current->in_execve == 1
> will cause this refcnt bug. You are simply ignoring that there is possibility
> that execve() fails.

you mean security_bprm_creds_for_exec() denying exec?
hmm. got it. refcnt model needs to change then.

> > Not true again.
> > usermode_blob is part of the kernel module.
> 
> Disagree.

Disagree with what? that blob is part of kernel module? huh?
what is it then?

> 
> > Kernel module when loaded doesn't have path.
> 
> Disagree.
> 
> Kernel modules can be trusted via module signature mechanism, and the byte array
> (which contains code / data) is protected by keeping that byte array within the
> kernel address space. Therefore, pathname based security does not need to complain
> that there is no pathname when kernel module is loaded.

I already explained upthread that blob is part of .rodata or .init.rodata
of kernel module and covered by the same signature mechanism.

> However, regarding usermode_blob, although the byte array (which contains code / data)
> might be initially loaded from the kernel space (which is protected), that byte array
> is no longer protected (e.g. SIGKILL, strace()) when executed because they are placed
> in the user address space. Thus, LSM modules (including pathname based security) want
> to control how that byte array can behave.

It's privileged memory regardless. root can poke into kernel or any process memory.

> On 2020/06/24 3:53, Eric W. Biederman wrote:
> > This isn't work anyone else can do because there are not yet any real in
> > tree users of fork_blob.  The fact that no one else can make
> > substantials changes to the code because it has no users is what gets in
> > the way of maintenance.
> 
> It sounds to me that fork_blob() is a dangerous interface which anonymously
> allows arbitrary behavior in an unprotected environment. Therefore,

I think you missed the part that user blob is signed as part of kernel module.

> > Either a path needs to be provided or the LSMs that work in terms
> > of paths need to be fixed.
> 
> LSM modules want to control how that byte array can behave. But Alexei
> still does not explain how information for LSM modules can be provided.

huh?
please see net/bpfilter/.

> 
> > My recomendation for long term maintenance is to split fork_blob into 2
> > functions: fs_from_blob, and the ordinary call_usermodehelper_exec.
> > That removes the need for any special support for anything in the exec
> > path because your blob will also have a path for your file, and the
> > file in the filesystem can be reused for restart.
> 
> Yes, that would be an approach for providing information for LSM modules.
> 
> > But with no in-tree users none of us can do anything bug guess what
> > the actual requirements of fork_usermode_blob are.
> 
> Exactly. Since it is not explained why the usermode process started by
> fork_usermode_blob() cannot interfere (or be interfered by) the rest of
> the system (including normal usermode processes), the byte array comes from
> the kernel address space is insufficient for convincing LSM modules to
> ignore what that byte array can do.

Sounds like tomoyo doesn't trust kernel modules. I don't think that is
fixable with any amount of explantation.