Re: [PATCH v9] fs: clear file privilege bits when mmap writing

From: Kees Cook
Date: Wed May 25 2016 - 17:37:11 EST

On Thu, Jan 14, 2016 at 9:55 PM, Konstantin Khlebnikov <koct9i@xxxxxxxxx> wrote:
> On Fri, Jan 15, 2016 at 12:22 AM, Kees Cook <keescook@xxxxxxxxxxxx> wrote:
>> Normally, when a user can modify a file that has setuid or setgid bits,
>> those bits are cleared when they are not the file owner or a member
>> of the group. This is enforced when using write and truncate but not
>> when writing to a shared mmap on the file. This could allow the file
>> writer to gain privileges by changing a binary without losing the
>> setuid/setgid/caps bits.
>>
>> Changing the bits requires holding inode->i_mutex, so it cannot be done
>> during the page fault (due to mmap_sem being held during the fault).
>> Instead, clear the bits if PROT_WRITE is being used at mmap open time,
>> or added at mprotect tLooks good to me.ime.
>>
>> Since we can't do the check in the right place inside mmap (due to
>> holding mmap_sem), we have to do it before holding mmap_sem, which
>> means duplicating some checks, which have to be available to the non-MMU
>> builds too.
>>
>> When walking VMAs during mprotect, we need to drop mmap_sem (while
>> holding a file reference) and restart the walk after clearing privileges.
>>
>> Signed-off-by: Kees Cook <keescook@xxxxxxxxxxxx>
>
> Looks good. Ack.

Hm, this didn't end up getting picked up. (This jumped out at me again
because i_mutex just vanished...)

Al, what's the right way to update the locking in this patch?

-Kees

>
>> ---
>> v9:
>> - use file_needs_remove_privs, jack & koct9i
>> v8:
>> - use mmap/mprotect method, with mprotect walk restart, thanks to koct9i
>> v7:
>> - document and avoid arch-specific O_* values, viro
>> v6:
>> - clarify ETXTBSY situation in comments, luto
>> v5:
>> - add to f_flags instead, viro
>> - add i_mutex during __fput, jack
>> v4:
>> - delay removal instead of still needing mmap_sem for mprotect, yalin
>> v3:
>> - move outside of mmap_sem for real now, fengguang
>> - check return code of file_remove_privs, akpm
>> v2:
>> - move to mmap from fault handler, jack
>> ---
>> include/linux/mm.h | 1 +
>> mm/mmap.c | 20 ++++----------------
>> mm/mprotect.c | 24 ++++++++++++++++++++++++
>> mm/util.c | 50 ++++++++++++++++++++++++++++++++++++++++++++++++++
>> 4 files changed, 79 insertions(+), 16 deletions(-)
>>
>> diff --git a/include/linux/mm.h b/include/linux/mm.h
>> index 00bad7793788..b264c8be7114 100644
>> --- a/include/linux/mm.h
>> +++ b/include/linux/mm.h
>> @@ -1912,6 +1912,7 @@ extern unsigned long get_unmapped_area(struct file *, unsigned long, unsigned lo
>>
>> extern unsigned long mmap_region(struct file *file, unsigned long addr,
>> unsigned long len, vm_flags_t vm_flags, unsigned long pgoff);
>> +extern int do_mmap_shared_checks(struct file *file, unsigned long prot);
>> extern unsigned long do_mmap(struct file *file, unsigned long addr,
>> unsigned long len, unsigned long prot, unsigned long flags,
>> vm_flags_t vm_flags, unsigned long pgoff, unsigned long *populate);
>> diff --git a/mm/mmap.c b/mm/mmap.c
>> index 2ce04a649f6b..b3424db0a29e 100644
>> --- a/mm/mmap.c
>> +++ b/mm/mmap.c
>> @@ -1320,25 +1320,13 @@ unsigned long do_mmap(struct file *file, unsigned long addr,
>> return -EAGAIN;
>>
>> if (file) {
>> - struct inode *inode = file_inode(file);
>> + int err;
>>
>> switch (flags & MAP_TYPE) {
>> case MAP_SHARED:
>> - if ((prot&PROT_WRITE) && !(file->f_mode&FMODE_WRITE))
>> - return -EACCES;
>> -
>> - /*
>> - * Make sure we don't allow writing to an append-only
>> - * file..
>> - */
>> - if (IS_APPEND(inode) && (file->f_mode & FMODE_WRITE))
>> - return -EACCES;
>> -
>> - /*
>> - * Make sure there are no mandatory locks on the file.
>> - */
>> - if (locks_verify_locked(file))
>> - return -EAGAIN;
>> + err = do_mmap_shared_checks(file, prot);
>> + if (err)
>> + return err;
>>
>> vm_flags |= VM_SHARED | VM_MAYSHARE;
>> if (!(file->f_mode & FMODE_WRITE))
>> diff --git a/mm/mprotect.c b/mm/mprotect.c
>> index ef5be8eaab00..57cb81c11668 100644
>> --- a/mm/mprotect.c
>> +++ b/mm/mprotect.c
>> @@ -12,6 +12,7 @@
>> #include <linux/hugetlb.h>
>> #include <linux/shm.h>
>> #include <linux/mman.h>
>> +#include <linux/file.h>
>> #include <linux/fs.h>
>> #include <linux/highmem.h>
>> #include <linux/security.h>
>> @@ -375,6 +376,7 @@ SYSCALL_DEFINE3(mprotect, unsigned long, start, size_t, len,
>>
>> vm_flags = calc_vm_prot_bits(prot);
>>
>> +restart:
>> down_write(&current->mm->mmap_sem);
>>
>> vma = find_vma(current->mm, start);
>> @@ -416,6 +418,28 @@ SYSCALL_DEFINE3(mprotect, unsigned long, start, size_t, len,
>> goto out;
>> }
>>
>> + /*
>> + * If we're adding write permissions to a shared file,
>> + * we must clear privileges (like done at mmap time),
>> + * but we have to juggle the locks to avoid holding
>> + * mmap_sem while holding i_mutex.
>> + */
>> + if ((vma->vm_flags & VM_SHARED) && vma->vm_file &&
>> + (newflags & VM_WRITE) && !(vma->vm_flags & VM_WRITE) &&
>> + file_needs_remove_privs(vma->vm_file)) {
>> + struct file *file = get_file(vma->vm_file);
>> +
>> + start = vma->vm_start;
>> + up_write(&current->mm->mmap_sem);
>> + mutex_lock(&file_inode(file)->i_mutex);
>> + error = file_remove_privs(file);
>> + mutex_unlock(&file_inode(file)->i_mutex);
>> + fput(file);
>> + if (error)
>> + return error;
>> + goto restart;
>> + }
>> +
>> error = security_file_mprotect(vma, reqprot, prot);
>> if (error)
>> goto out;
>> diff --git a/mm/util.c b/mm/util.c
>> index 9af1c12b310c..1882eaf33a37 100644
>> --- a/mm/util.c
>> +++ b/mm/util.c
>> @@ -283,6 +283,29 @@ int __weak get_user_pages_fast(unsigned long start,
>> }
>> EXPORT_SYMBOL_GPL(get_user_pages_fast);
>>
>> +int do_mmap_shared_checks(struct file *file, unsigned long prot)
>> +{
>> + struct inode *inode = file_inode(file);
>> +
>> + if ((prot & PROT_WRITE) && !(file->f_mode & FMODE_WRITE))
>> + return -EACCES;
>> +
>> + /*
>> + * Make sure we don't allow writing to an append-only
>> + * file..
>> + */
>> + if (IS_APPEND(inode) && (file->f_mode & FMODE_WRITE))
>> + return -EACCES;
>> +
>> + /*
>> + * Make sure there are no mandatory locks on the file.
>> + */
>> + if (locks_verify_locked(file))
>> + return -EAGAIN;
>> +
>> + return 0;
>> +}
>> +
>> unsigned long vm_mmap_pgoff(struct file *file, unsigned long addr,
>> unsigned long len, unsigned long prot,
>> unsigned long flag, unsigned long pgoff)
>> @@ -291,6 +314,33 @@ unsigned long vm_mmap_pgoff(struct file *file, unsigned long addr,
>> struct mm_struct *mm = current->mm;
>> unsigned long populate;
>>
>> + /*
>> + * If we must remove privs, we do it here since doing it during
>> + * page fault may be expensive and cannot hold inode->i_mutex,
>> + * since mm->mmap_sem is already held.
>> + */
>> + if (file && (flag & MAP_TYPE) == MAP_SHARED && (prot & PROT_WRITE)) {
>> + struct inode *inode = file_inode(file);
>> + int err;
>> +
>> + if (!IS_NOSEC(inode)) {
>> + /*
>> + * Make sure we can't strip privs from a file that
>> + * wouldn't otherwise be allowed to be mmapped.
>> + */
>> + err = do_mmap_shared_checks(file, prot);
>> + if (err)
>> + return err;
>> +
>> + mutex_lock(&inode->i_mutex);
>> + err = file_remove_privs(file);
>> + mutex_unlock(&inode->i_mutex);
>> +
>> + if (err)
>> + return err;
>> + }
>> + }
>> +
>> ret = security_mmap_file(file, prot, flag);
>> if (!ret) {
>> down_write(&mm->mmap_sem);
>> --
>> 2.6.3
>>
>>
>> --
>> Kees Cook
>> Chrome OS & Brillo Security



--
Kees Cook
Chrome OS & Brillo Security