| Message ID | 1562964097-8578-1-git-send-email-zohar@linux.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [RFC] ima: fix ima_file_mmap circular locking dependency | expand |
Hi Mimi, On Fri, Jul 12, 2019 at 04:41:37PM -0400, Mimi Zohar wrote: > The LSM security_mmap_file hook is called before the mmap_sem is taken. > This results in IMA taking the i_mutex before the mmap_sem, yet the > normal locking order is mmap_sem, i_mutex. > > To resolve this problem, rename and call ima_mmap_file() after taking > the mmap_sem. > I don't think this is correct. The normal order is i_mutex, then mmap_sem. E.g., for buffered writes i_mutex is taken, then when each page is written the page may have to be faulted into memory which takes mmap_sem. What seems to have happened is that due to your patch "ima: verify mprotect change is consistent with mmap policy" which was in linux-next for a while, syzbot found a reproducer on next-20190531 for "possible deadlock in process_measurement" (https://lkml.kernel.org/lkml/00000000000054e5d1058a6df2eb@google.com/), which already had an open syzbot report for some other reason, possibly overlayfs-related. The same mprotect issue also got reported in 2 other syzbot reports, "possible deadlock in get_user_pages_unlocked (2)" and "possible deadlock in __do_page_fault (2)". Since your patch was dropped from linux-next, the issue no longer exists. I invalidated the other 2 reports for you but I didn't notice this one because it was a much older syzbot report. So I suggest just invalidating the report "possible deadlock in process_measurement" too, unless you think you think the older overlayfs-related deadlock report is still valid and actionable. It doesn't have a reproducer and was last seen 5 months ago, so it *might* be stale: https://syzkaller.appspot.com/text?tag=CrashReport&x=1767eeef400000 - Eric
Hi Eric, On Fri, 2019-07-12 at 16:13 -0700, Eric Biggers wrote: > So I suggest just invalidating the report "possible deadlock in > process_measurement" too, unless you think you think the older overlayfs-related > deadlock report is still valid and actionable. It doesn't have a reproducer > and was last seen 5 months ago, so it *might* be stale: > https://syzkaller.appspot.com/text?tag=CrashReport&x=1767eeef400000 Yes, please invalidate that one as well. Mimi
diff --git a/include/linux/ima.h b/include/linux/ima.h index 71796a0959d9..10adb38e0e43 100644 --- a/include/linux/ima.h +++ b/include/linux/ima.h @@ -17,7 +17,7 @@ extern int ima_bprm_check(struct linux_binprm *bprm); extern int ima_file_check(struct file *file, int mask); extern void ima_post_create_tmpfile(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_mmap_file(struct file *file, unsigned long prot); extern int ima_load_data(enum kernel_load_data_id id); extern int ima_read_file(struct file *file, enum kernel_read_file_id id); extern int ima_post_read_file(struct file *file, void *buf, loff_t size, @@ -65,7 +65,7 @@ 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_mmap_file(struct file *file, unsigned long prot) { return 0; } diff --git a/ipc/shm.c b/ipc/shm.c index ce1ca9f7c6e9..a712c7d426f0 100644 --- a/ipc/shm.c +++ b/ipc/shm.c @@ -34,6 +34,7 @@ #include <linux/mman.h> #include <linux/shmem_fs.h> #include <linux/security.h> +#include <linux/ima.h> #include <linux/syscalls.h> #include <linux/audit.h> #include <linux/capability.h> @@ -1549,6 +1550,10 @@ long do_shmat(int shmid, char __user *shmaddr, int shmflg, goto out_fput; } + err = ima_mmap_file(file, prot); + if (err) + goto out_fput; + if (addr && !(shmflg & SHM_REMAP)) { err = -EINVAL; if (addr + size < addr) diff --git a/mm/util.c b/mm/util.c index 9834c4ab7d8e..dbf2c15caacd 100644 --- a/mm/util.c +++ b/mm/util.c @@ -9,6 +9,7 @@ #include <linux/sched/mm.h> #include <linux/sched/task_stack.h> #include <linux/security.h> +#include <linux/ima.h> #include <linux/swap.h> #include <linux/swapops.h> #include <linux/mman.h> @@ -360,6 +361,13 @@ unsigned long vm_mmap_pgoff(struct file *file, unsigned long addr, if (!ret) { if (down_write_killable(&mm->mmap_sem)) return -EINTR; + + ret = ima_mmap_file(file, prot); + if (ret) { + up_write(&mm->mmap_sem); + return ret; + } + ret = do_mmap_pgoff(file, addr, len, prot, flag, pgoff, &populate, &uf); up_write(&mm->mmap_sem); diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c index 45d9ece88668..14678665cdc8 100644 --- a/security/integrity/ima/ima_main.c +++ b/security/integrity/ima/ima_main.c @@ -380,7 +380,7 @@ 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_mmap_file(struct file *file, unsigned long prot) { u32 secid; diff --git a/security/security.c b/security/security.c index a749d884faec..e324c425e466 100644 --- a/security/security.c +++ b/security/security.c @@ -1410,12 +1410,8 @@ 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) { - int ret; - ret = call_int_hook(mmap_file, 0, file, prot, - mmap_prot(file, prot), flags); - if (ret) - return ret; - return ima_file_mmap(file, prot); + return call_int_hook(mmap_file, 0, file, prot, + mmap_prot(file, prot), flags); } int security_mmap_addr(unsigned long addr)
The LSM security_mmap_file hook is called before the mmap_sem is taken. This results in IMA taking the i_mutex before the mmap_sem, yet the normal locking order is mmap_sem, i_mutex. To resolve this problem, rename and call ima_mmap_file() after taking the mmap_sem. Reported-by: syzbot+5ab61747675a87ea359d@syzkaller.appspotmail.com Signed-off-by: Mimi Zohar <zohar@linux.ibm.com> --- include/linux/ima.h | 4 ++-- ipc/shm.c | 5 +++++ mm/util.c | 8 ++++++++ security/integrity/ima/ima_main.c | 2 +- security/security.c | 8 ++------ 5 files changed, 18 insertions(+), 9 deletions(-)