Re: [PATCH 1/3] ima: Remove inode lock
From: Lorenzo Stoakes <hidden>
Date: 2024-10-18 11:01:01
Also in:
bpf, linux-fsdevel, linux-integrity, linux-mm, lkml
+ Liam, Jann On Fri, Oct 18, 2024 at 01:49:06PM +0300, Kirill A. Shutemov wrote:
quoted hunk ↗ jump to hunk
On Fri, Oct 18, 2024 at 11:24:06AM +0200, Roberto Sassu wrote:quoted
Probably it is hard, @Kirill would there be any way to safely move security_mmap_file() out of the mmap_lock lock?What about something like this (untested):diff --git a/mm/mmap.c b/mm/mmap.c index dd4b35a25aeb..03473e77d356 100644 --- a/mm/mmap.c +++ b/mm/mmap.c@@ -1646,6 +1646,26 @@ SYSCALL_DEFINE5(remap_file_pages, unsigned long, start, unsigned long, size, if (pgoff + (size >> PAGE_SHIFT) < pgoff) return ret; + if (mmap_read_lock_killable(mm)) + return -EINTR; + + vma = vma_lookup(mm, start); + + if (!vma || !(vma->vm_flags & VM_SHARED)) { + mmap_read_unlock(mm); + return -EINVAL; + } + + file = get_file(vma->vm_file); + + mmap_read_unlock(mm); + + ret = security_mmap_file(vma->vm_file, prot, flags);
Accessing VMA fields without any kind of lock is... very much not advised. I'm guessing you meant to say: ret = security_mmap_file(file, prot, flags); Here? :) I see the original code did this, but obviously was under an mmap lock. I guess given you check that the file is the same below this.... should be fine? Assuming nothing can come in and invalidate the security_mmap_file() check in the mean time somehow? Jann any thoughts?
quoted hunk ↗ jump to hunk
+ if (ret) { + fput(file); + return ret; + } + if (mmap_write_lock_killable(mm)) return -EINTR;@@ -1654,6 +1674,9 @@ SYSCALL_DEFINE5(remap_file_pages, unsigned long, start, unsigned long, size, if (!vma || !(vma->vm_flags & VM_SHARED)) goto out; + if (vma->vm_file != file) + goto out; + if (start + size > vma->vm_end) { VMA_ITERATOR(vmi, mm, vma->vm_end); struct vm_area_struct *next, *prev = vma;@@ -1688,16 +1711,11 @@ SYSCALL_DEFINE5(remap_file_pages, unsigned long, start, unsigned long, size, if (vma->vm_flags & VM_LOCKED) flags |= MAP_LOCKED; - file = get_file(vma->vm_file); - ret = security_mmap_file(vma->vm_file, prot, flags); - if (ret) - goto out_fput; ret = do_mmap(vma->vm_file, start, size, prot, flags, 0, pgoff, &populate, NULL); -out_fput: - fput(file); out: mmap_write_unlock(mm); + fput(file); if (populate) mm_populate(ret, populate); if (!IS_ERR_VALUE(ret)) -- Kiryl Shutsemau / Kirill A. Shutemov