Re: [PATCH v2] security: Restore passing final prot to ima_file_mmap()

[Roberto Sassu]

On Wed, 2023-01-11 at 09:25 -0500, Paul Moore wrote:
> On Wed, Jan 11, 2023 at 4:31 AM Roberto Sassu
> <roberto.sassu@xxxxxxxxxxxxxxx> wrote:
> > On Fri, 2023-01-06 at 16:14 -0500, Paul Moore wrote:
> > > On Wed, Dec 21, 2022 at 9:10 AM Roberto Sassu
> > > <roberto.sassu@xxxxxxxxxxxxxxx> wrote:
> > > > From: Roberto Sassu <roberto.sassu@xxxxxxxxxx>
> > > > 
> > > > Commit 98de59bfe4b2f ("take calculation of final prot in
> > > > security_mmap_file() into a helper") moved the code to update prot with the
> > > > actual protection flags to be granted to the requestor by the kernel to a
> > > > helper called mmap_prot(). However, the patch didn't update the argument
> > > > passed to ima_file_mmap(), making it receive the requested prot instead of
> > > > the final computed prot.
> > > > 
> > > > A possible consequence is that files mmapped as executable might not be
> > > > measured/appraised if PROT_EXEC is not requested but subsequently added in
> > > > the final prot.
> > > > 
> > > > Replace prot with mmap_prot(file, prot) as the second argument of
> > > > ima_file_mmap() to restore the original behavior.
> > > > 
> > > > Cc: stable@xxxxxxxxxxxxxxx
> > > > Fixes: 98de59bfe4b2 ("take calculation of final prot in security_mmap_file() into a helper")
> > > > Signed-off-by: Roberto Sassu <roberto.sassu@xxxxxxxxxx>
> > > > ---
> > > >  security/security.c | 2 +-
> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > 
> > > > diff --git a/security/security.c b/security/security.c
> > > > index d1571900a8c7..0d2359d588a1 100644
> > > > --- a/security/security.c
> > > > +++ b/security/security.c
> > > > @@ -1666,7 +1666,7 @@ int security_mmap_file(struct file *file, unsigned long prot,
> > > >                                         mmap_prot(file, prot), flags);
> > > >         if (ret)
> > > >                 return ret;
> > > > -       return ima_file_mmap(file, prot);
> > > > +       return ima_file_mmap(file, mmap_prot(file, prot));
> > > >  }
> > > 
> > > This seems like a reasonable fix, although as the original commit is
> > > ~10 years old at this point I am a little concerned about the impact
> > > this might have on IMA.  Mimi, what do you think?

As a user, I probably would like to know that my system is not
measuring what it is supposed to measure. The rule:

measure func=MMAP_CHECK mask=MAY_EXEC

is looking for executable code mapped in memory. If it is requested by
the application or the kernel, probably it does not make too much
difference from the perspective of measurement goals.

If we add a new policy keyword, existing policies would not be updated
unless the system administrator notices it. If a remote attestation
fails, the administrator has to look into it.

Maybe we can introduce a new hook called MMAP_CHECK_REQ, so that an
administrator could change the policy to have the current behavior, if
the administrator wishes so.

Roberto

> > > Beyond that, my only other comment would be to only call mmap_prot()
> > > once and cache the results in a local variable.  You could also fix up
> > > some of the ugly indentation crimes in security_mmap_file() while you
> > > are at it, e.g. something like this:
> > 
> > Hi Paul
> > 
> > thanks for the comments. With the patch set to move IMA and EVM to the
> > LSM infrastructure we will be back to calling mmap_prot() only once,
> > but I guess we could do anyway, as the patch (if accepted) would be
> > likely backported to stable kernels.
> 
> I think there is value in fixing this now and keeping it separate from
> the IMA-to-LSM work as they really are disjoint.
>