On Thu, 2023-01-26 at 14:37 -0500, Stefan Berger wrote:
>
> On 1/26/23 11:38, Roberto Sassu 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, to be
> > the actual protections applied to the kernel, to a new helper called
> > mmap_prot().
> >
> > However, while without the helper ima_file_mmap() was getting the updated
> > prot, with the helper ima_file_mmap() gets the original prot, which
> > contains the protections requested by the application.
> >
> > A possible consequence of this change is that, if an application calls
> > mmap() with only PROT_READ, and the kernel applies PROT_EXEC in addition,
> > that application would have access to executable memory without having this
> > event recorded in the IMA measurement list. This situation would occur for
> > example if the application, before mmap(), calls the personality() system
> > call with READ_IMPLIES_EXEC as the first argument.
> >
> > Align ima_file_mmap() parameters with those of the mmap_file LSM hook, so
> > that IMA can receive both the requested prot and the final prot. Since the
> > requested protections are stored in a new variable, and the final
> > protections are stored in the existing variable, this effectively restores
> > the original behavior of the MMAP_CHECK hook.
> >
> And flags is being passed in preparation for IMA to meet the interface
> requirements of the LSM hooks - I suppose in preparation for IMA to become an LSM.
Yes, correct. I reused a patch from that patch set. Anyway reqprot was
needed for MMAP_CHECK_REQPROT.
> > 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>
>
> Reviewed-by: Stefan Berger <stefanb@xxxxxxxxxxxxx>
Thanks
Roberto
> > ---
> > include/linux/ima.h | 6 ++++--
> > security/integrity/ima/ima_main.c | 7 +++++--
> > security/security.c | 7 ++++---
> > 3 files changed, 13 insertions(+), 7 deletions(-)
> >
> > diff --git a/include/linux/ima.h b/include/linux/ima.h
> > index 5a0b2a285a18..d79fee67235e 100644
> > --- a/include/linux/ima.h
> > +++ b/include/linux/ima.h
> > @@ -21,7 +21,8 @@ extern int ima_file_check(struct file *file, int mask);
> > extern void ima_post_create_tmpfile(struct user_namespace *mnt_userns,
> > struct inode *inode);
> > extern void ima_file_free(struct file *file);
> > -extern int ima_file_mmap(struct file *file, unsigned long prot);
> > +extern int ima_file_mmap(struct file *file, unsigned long reqprot,
> > + unsigned long prot, unsigned long flags);
> > extern int ima_file_mprotect(struct vm_area_struct *vma, unsigned long prot);
> > extern int ima_load_data(enum kernel_load_data_id id, bool contents);
> > extern int ima_post_load_data(char *buf, loff_t size,
> > @@ -76,7 +77,8 @@ static inline void ima_file_free(struct file *file)
> > return;
> > }
> >
> > -static inline int ima_file_mmap(struct file *file, unsigned long prot)
> > +static inline int ima_file_mmap(struct file *file, unsigned long reqprot,
> > + unsigned long prot, unsigned long flags)
> > {
> > return 0;
> > }
> > diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
> > index 377300973e6c..f48f4e694921 100644
> > --- a/security/integrity/ima/ima_main.c
> > +++ b/security/integrity/ima/ima_main.c
> > @@ -397,7 +397,9 @@ static int process_measurement(struct file *file, const struct cred *cred,
> > /**
> > * ima_file_mmap - based on policy, collect/store measurement.
> > * @file: pointer to the file to be measured (May be NULL)
> > - * @prot: contains the protection that will be applied by the kernel.
> > + * @reqprot: protection requested by the application
> > + * @prot: protection that will be applied by the kernel
> > + * @flags: operational flags
> > *
> > * Measure files being mmapped executable based on the ima_must_measure()
> > * policy decision.
> > @@ -405,7 +407,8 @@ static int process_measurement(struct file *file, const struct cred *cred,
> > * On success return 0. On integrity appraisal error, assuming the file
> > * is in policy and IMA-appraisal is in enforcing mode, return -EACCES.
> > */
> > -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)
> > {
> > u32 secid;
> >
> > diff --git a/security/security.c b/security/security.c
> > index d1571900a8c7..174afa4fad81 100644
> > --- a/security/security.c
> > +++ b/security/security.c
> > @@ -1661,12 +1661,13 @@ static inline unsigned long mmap_prot(struct file *file, unsigned long prot)
> > int security_mmap_file(struct file *file, unsigned long prot,
> > unsigned long flags)
> > {
> > + unsigned long prot_adj = mmap_prot(file, prot);
> > int ret;
> > - ret = call_int_hook(mmap_file, 0, file, prot,
> > - mmap_prot(file, prot), flags);
> > +
> > + 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, prot_adj, flags);
> > }
> >
> > int security_mmap_addr(unsigned long addr)
