| Message ID | 20230126163812.1870942-1-roberto.sassu@huaweicloud.com (mailing list archive) |
|---|---|
| State | Handled Elsewhere |
| Headers | show |
| Series | [v3,1/2] ima: Align ima_file_mmap() parameters with mmap_file LSM hook | expand |
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. > 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> > --- > 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)
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)
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)