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

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

[Roberto Sassu]

From: Roberto Sassu <roberto.sassu@huawei.com>

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@vger.kernel.org
Fixes: 98de59bfe4b2 ("take calculation of final prot in security_mmap_file() into a helper")
Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
---
 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));
 }
 
 int security_mmap_addr(unsigned long addr)
-- 
2.25.1

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

[Paul Moore]

On Wed, Dec 21, 2022 at 9:10 AM Roberto Sassu
<roberto.sassu@huaweicloud.com> wrote:
>
> From: Roberto Sassu <roberto.sassu@huawei.com>
>
> 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@vger.kernel.org
> Fixes: 98de59bfe4b2 ("take calculation of final prot in security_mmap_file() into a helper")
> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> ---
>  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?

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:

diff --git a/security/security.c b/security/security.c
index d1571900a8c7..2f9cad9ecac8 100644
--- a/security/security.c
+++ b/security/security.c
@@ -1662,11 +1662,12 @@ int security_mmap_file(struct file *file, unsigned long
prot,
                       unsigned long flags)
{
       int ret;
-       ret = call_int_hook(mmap_file, 0, file, prot,
-                                       mmap_prot(file, prot), flags);
+       unsigned long prot_adj = mmap_prot(file, prot);
+
+       ret = call_int_hook(mmap_file, 0, file, prot, prot_adj, flags);
       if (ret)
               return ret;
-       return ima_file_mmap(file, prot);
+       return ima_file_mmap(file, prot_adj);
}

--
paul-moore.com

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

[Roberto Sassu]

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@huaweicloud.com> wrote:
> > From: Roberto Sassu <roberto.sassu@huawei.com>
> > 
> > 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@vger.kernel.org
> > Fixes: 98de59bfe4b2 ("take calculation of final prot in security_mmap_file() into a helper")
> > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> > ---
> >  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?
> 
> 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.

Thanks

Roberto

> diff --git a/security/security.c b/security/security.c
> index d1571900a8c7..2f9cad9ecac8 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -1662,11 +1662,12 @@ int security_mmap_file(struct file *file, unsigned long
> prot,
>                        unsigned long flags)
> {
>        int ret;
> -       ret = call_int_hook(mmap_file, 0, file, prot,
> -                                       mmap_prot(file, prot), flags);
> +       unsigned long prot_adj = mmap_prot(file, prot);
> +
> +       ret = call_int_hook(mmap_file, 0, file, prot, prot_adj, flags);
>        if (ret)
>                return ret;
> -       return ima_file_mmap(file, prot);
> +       return ima_file_mmap(file, prot_adj);
> }
> 
> --
> paul-moore.com

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

[Paul Moore]

On Wed, Jan 11, 2023 at 4:31 AM Roberto Sassu
<roberto.sassu@huaweicloud.com> 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@huaweicloud.com> wrote:
> > > From: Roberto Sassu <roberto.sassu@huawei.com>
> > >
> > > 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@vger.kernel.org
> > > Fixes: 98de59bfe4b2 ("take calculation of final prot in security_mmap_file() into a helper")
> > > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> > > ---
> > >  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?
> >
> > 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.

-- 
paul-moore.com

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@huaweicloud.com> 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@huaweicloud.com> wrote:
> > > > From: Roberto Sassu <roberto.sassu@huawei.com>
> > > > 
> > > > 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@vger.kernel.org
> > > > Fixes: 98de59bfe4b2 ("take calculation of final prot in security_mmap_file() into a helper")
> > > > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> > > > ---
> > > >  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.
> 

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

[Mimi Zohar]

On Thu, 2023-01-12 at 13:36 +0100, Roberto Sassu wrote:
> 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@huaweicloud.com> 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@huaweicloud.com> wrote:
> > > > > From: Roberto Sassu <roberto.sassu@huawei.com>
> > > > > 
> > > > > 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@vger.kernel.org
> > > > > Fixes: 98de59bfe4b2 ("take calculation of final prot in security_mmap_file() into a helper")
> > > > > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> > > > > ---
> > > > >  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:

Agreed, that it is measuring what it is supposed to measure.

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

Currently, it's limited to measuring file's being mmapped.  From what I
can tell from looking at the code, additional measurements would be
included when "current->personality & READ_IMPLIES_EXEC".

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

Verifying the measurement list against a TPM quote should work
regardless of additional measurements.  The attestation server,
however, should also check for unknown files.

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

Agreed, for backwards compatibility this would be good.  Would you
support it afterward transitioning IMA to an LSM?

However "_REQ" could mean either requested or required.

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

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

[Roberto Sassu]

On Thu, 2023-01-12 at 12:45 -0500, Mimi Zohar wrote:
> On Thu, 2023-01-12 at 13:36 +0100, Roberto Sassu wrote:
> > 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@huaweicloud.com> 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@huaweicloud.com> wrote:
> > > > > > From: Roberto Sassu <roberto.sassu@huawei.com>
> > > > > > 
> > > > > > 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@vger.kernel.org
> > > > > > Fixes: 98de59bfe4b2 ("take calculation of final prot in security_mmap_file() into a helper")
> > > > > > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> > > > > > ---
> > > > > >  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:
> 
> Agreed, that it is measuring what it is supposed to measure.
> 
> > 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.
> 
> Currently, it's limited to measuring file's being mmapped.  From what I
> can tell from looking at the code, additional measurements would be
> included when "current->personality & READ_IMPLIES_EXEC".

Yes, I developed a small program to see the differences:

void main()
{
        struct stat st;
        personality(READ_IMPLIES_EXEC);
        stat("test-file", &st);
        int fd = open("test-file", O_RDONLY);
        mmap(0, st.st_size, PROT_READ, MAP_PRIVATE, fd, 0);
}

Without the patch, the test-file measurement does not appear.

> > 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.
> 
> Verifying the measurement list against a TPM quote should work
> regardless of additional measurements.  The attestation server,
> however, should also check for unknown files.
> 
> > 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.
> 
> Agreed, for backwards compatibility this would be good.  Would you
> support it afterward transitioning IMA to an LSM?

Yes, I have a patch to align ima_file_mmap() with the mmap_file() hook
definition:

-int ima_file_mmap(struct file *file, unsigned long prot)
+int ima_file_mmap(struct file *file, unsigned long reqprot,
+		  unsigned long prot, unsigned long flags)

This would have also fixed the issue. But for backporting, I did a
standalone patch.

I noticed that Kees found this as well:

-int ima_file_mmap(struct file *file, unsigned long prot)
+static int ima_file_mmap(struct file *file, unsigned long reqprot,
+			 unsigned long prot, unsigned long flags)
 {
 	u32 secid;
 
-	if (file && (prot & PROT_EXEC)) {
+	if (file && (reqprot & PROT_EXEC)) {

but from the history I saw that the original intent was to consider
prot, not reqprot.

> However "_REQ" could mean either requested or required.

It was to recall reqprot. I could rename to MMAP_CHECK_REQPROT.

Thanks

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

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

[Paul Moore]

On Fri, Jan 13, 2023 at 5:53 AM Roberto Sassu
<roberto.sassu@huaweicloud.com> wrote:
> On Thu, 2023-01-12 at 12:45 -0500, Mimi Zohar wrote:
> > On Thu, 2023-01-12 at 13:36 +0100, Roberto Sassu wrote:
> > > 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@huaweicloud.com> 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@huaweicloud.com> wrote:
> > > > > > > From: Roberto Sassu <roberto.sassu@huawei.com>
> > > > > > >
> > > > > > > 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@vger.kernel.org
> > > > > > > Fixes: 98de59bfe4b2 ("take calculation of final prot in security_mmap_file() into a helper")
> > > > > > > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> > > > > > > ---
> > > > > > >  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?

So ... where do we stand on this patch, Mimi, Roberto?  I stand by my
original comment, but I would want to see an ACK from Mimi at the very
least before merging this upstream.  If this isn't ACK-able, do we
have a plan to resolve this soon?

-- 
paul-moore.com

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

[Roberto Sassu]

On Fri, 2023-01-20 at 16:04 -0500, Paul Moore wrote:
> On Fri, Jan 13, 2023 at 5:53 AM Roberto Sassu
> <roberto.sassu@huaweicloud.com> wrote:
> > On Thu, 2023-01-12 at 12:45 -0500, Mimi Zohar wrote:
> > > On Thu, 2023-01-12 at 13:36 +0100, Roberto Sassu wrote:
> > > > 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@huaweicloud.com> 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@huaweicloud.com> wrote:
> > > > > > > > From: Roberto Sassu <roberto.sassu@huawei.com>
> > > > > > > > 
> > > > > > > > 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@vger.kernel.org
> > > > > > > > Fixes: 98de59bfe4b2 ("take calculation of final prot in security_mmap_file() into a helper")
> > > > > > > > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> > > > > > > > ---
> > > > > > > >  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?
> 
> So ... where do we stand on this patch, Mimi, Roberto?  I stand by my
> original comment, but I would want to see an ACK from Mimi at the very
> least before merging this upstream.  If this isn't ACK-able, do we
> have a plan to resolve this soon?

Sorry, I had business trips last week. Will send the patches this week.

Roberto

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

[Paul Moore]

On Mon, Jan 23, 2023 at 3:30 AM Roberto Sassu
<roberto.sassu@huaweicloud.com> wrote:
> On Fri, 2023-01-20 at 16:04 -0500, Paul Moore wrote:
> > On Fri, Jan 13, 2023 at 5:53 AM Roberto Sassu
> > <roberto.sassu@huaweicloud.com> wrote:
> > > On Thu, 2023-01-12 at 12:45 -0500, Mimi Zohar wrote:
> > > > On Thu, 2023-01-12 at 13:36 +0100, Roberto Sassu wrote:
> > > > > 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@huaweicloud.com> 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@huaweicloud.com> wrote:
> > > > > > > > > From: Roberto Sassu <roberto.sassu@huawei.com>
> > > > > > > > >
> > > > > > > > > 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@vger.kernel.org
> > > > > > > > > Fixes: 98de59bfe4b2 ("take calculation of final prot in security_mmap_file() into a helper")
> > > > > > > > > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> > > > > > > > > ---
> > > > > > > > >  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?
> >
> > So ... where do we stand on this patch, Mimi, Roberto?  I stand by my
> > original comment, but I would want to see an ACK from Mimi at the very
> > least before merging this upstream.  If this isn't ACK-able, do we
> > have a plan to resolve this soon?
>
> Sorry, I had business trips last week. Will send the patches this week.

No worries, I just wasn't sure of the status and wanted to check in on this.

-- 
paul-moore.com

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

[Mimi Zohar]

On Fri, 2023-01-13 at 11:52 +0100, Roberto Sassu wrote:

> > > 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.
> > 
> > Verifying the measurement list against a TPM quote should work
> > regardless of additional measurements.  The attestation server,
> > however, should also check for unknown files.
> > 
> > > 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.

<snip>

> > However "_REQ" could mean either requested or required.
> 
> It was to recall reqprot. I could rename to MMAP_CHECK_REQPROT.

That sounds good.

-- 
thanks,

Mimib