| Message ID | 20221221141007.2579770-1-roberto.sassu@huaweicloud.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] security: Restore passing final prot to ima_file_mmap() | expand |
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
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
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.
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. >
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. > > >
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. > > >
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?
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.
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
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.
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)