Message-ID: <34e99a8326316f998d034b009469589ca177769d.camel@huaweicloud.com>
Subject: Re: [PATCH v3 1/2] ima: Align ima_file_mmap() parameters with
 mmap_file LSM hook
From:   Roberto Sassu <roberto.sassu@huaweicloud.com>
To:     Stefan Berger <stefanb@linux.ibm.com>, zohar@linux.ibm.com,
        dmitry.kasatkin@gmail.com, jmorris@namei.org, serge@hallyn.com
Cc:     linux-integrity@vger.kernel.org,
        linux-security-module@vger.kernel.org,
        linux-kernel@vger.kernel.org, viro@zeniv.linux.org.uk,
        Roberto Sassu <roberto.sassu@huawei.com>,
        stable@vger.kernel.org
Date:   Fri, 27 Jan 2023 08:55:53 +0100
In-Reply-To: <295c5915-b680-16be-3d51-b7c1d2ca5e4f@linux.ibm.com>
References: <20230126163812.1870942-1-roberto.sassu@huaweicloud.com>
         <295c5915-b680-16be-3d51-b7c1d2ca5e4f@linux.ibm.com>
Content-Type: text/plain; charset="UTF-8"
User-Agent: Evolution 3.36.5-0ubuntu1 
MIME-Version: 1.0
Content-Transfer-Encoding: 7bit
Precedence: bulk

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@huawei.com>
> > 
> > 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@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>
> 
> Reviewed-by: Stefan Berger <stefanb@linux.ibm.com>

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)