* [PATCH v9] fs: clear file privilege bits when mmap writing @ 2016-01-14 21:22 Kees Cook 2016-01-15 5:55 ` Konstantin Khlebnikov 0 siblings, 1 reply; 8+ messages in thread From: Kees Cook @ 2016-01-14 21:22 UTC (permalink / raw) To: Andrew Morton Cc: Alexander Viro, Konstantin Khlebnikov, Andy Lutomirski, Jan Kara, yalin wang, Willy Tarreau, linux-mm, linux-kernel 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 time. 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@chromium.org> --- 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(¤t->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(¤t->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 -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@kvack.org. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a> ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH v9] fs: clear file privilege bits when mmap writing 2016-01-14 21:22 [PATCH v9] fs: clear file privilege bits when mmap writing Kees Cook @ 2016-01-15 5:55 ` Konstantin Khlebnikov 2016-01-15 6:18 ` Andy Lutomirski 2016-05-25 21:36 ` Kees Cook 0 siblings, 2 replies; 8+ messages in thread From: Konstantin Khlebnikov @ 2016-01-15 5:55 UTC (permalink / raw) To: Kees Cook Cc: Andrew Morton, Alexander Viro, Andy Lutomirski, Jan Kara, yalin wang, Willy Tarreau, linux-mm@kvack.org, Linux Kernel Mailing List On Fri, Jan 15, 2016 at 12:22 AM, Kees Cook <keescook@chromium.org> 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@chromium.org> Looks good. Ack. > --- > 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(¤t->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(¤t->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 -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@kvack.org. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a> ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v9] fs: clear file privilege bits when mmap writing 2016-01-15 5:55 ` Konstantin Khlebnikov @ 2016-01-15 6:18 ` Andy Lutomirski 2016-01-15 6:36 ` Konstantin Khlebnikov 2016-05-25 21:36 ` Kees Cook 1 sibling, 1 reply; 8+ messages in thread From: Andy Lutomirski @ 2016-01-15 6:18 UTC (permalink / raw) To: Konstantin Khlebnikov Cc: Kees Cook, Andrew Morton, Alexander Viro, Jan Kara, yalin wang, Willy Tarreau, linux-mm@kvack.org, Linux Kernel Mailing List On Thu, Jan 14, 2016 at 9:55 PM, Konstantin Khlebnikov <koct9i@gmail.com> wrote: > On Fri, Jan 15, 2016 at 12:22 AM, Kees Cook <keescook@chromium.org> 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@chromium.org> > > Looks good. Ack. While we're at it: int should_remove_suid(struct dentry *dentry) { umode_t mode = d_inode(dentry)->i_mode; int kill = 0; /* suid always must be killed */ if (unlikely(mode & S_ISUID)) kill = ATTR_KILL_SUID; /* * sgid without any exec bits is just a mandatory locking mark; leave * it alone. If some exec bits are set, it's a real sgid; kill it. */ if (unlikely((mode & S_ISGID) && (mode & S_IXGRP))) kill |= ATTR_KILL_SGID; if (unlikely(kill && !capable(CAP_FSETID) && S_ISREG(mode))) return kill; return 0; } EXPORT_SYMBOL(should_remove_suid); Oh wait, is that an implicit use of current_cred in vfs_write? No, it couldn't be. Kernel developers *never* make that mistake. This is, of course, totally fucked because this function doesn't have access to a struct file and therefore can't see f_cred. I'm not going to look in to this right now, but I swear I saw an exploit that took advantage of this bug recently. Anyone want to try to fix it? FWIW, posix says (man 3p write): Upon successful completion, where nbyte is greater than 0, write() shall mark for update the last data modification and last file status change timestamps of the file, and if the file is a regular file, the S_ISUID and S_ISGID bits of the file mode may be cleared. so maybe the thing to do is just drop the capable check entirely and cross our fingers that nothing was relying on it. --Andy > >> --- >> 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(¤t->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(¤t->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 -- Andy Lutomirski AMA Capital Management, LLC -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@kvack.org. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a> ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v9] fs: clear file privilege bits when mmap writing 2016-01-15 6:18 ` Andy Lutomirski @ 2016-01-15 6:36 ` Konstantin Khlebnikov 0 siblings, 0 replies; 8+ messages in thread From: Konstantin Khlebnikov @ 2016-01-15 6:36 UTC (permalink / raw) To: Andy Lutomirski Cc: Kees Cook, Andrew Morton, Alexander Viro, Jan Kara, yalin wang, Willy Tarreau, linux-mm@kvack.org, Linux Kernel Mailing List On Fri, Jan 15, 2016 at 9:18 AM, Andy Lutomirski <luto@amacapital.net> wrote: > On Thu, Jan 14, 2016 at 9:55 PM, Konstantin Khlebnikov <koct9i@gmail.com> wrote: >> On Fri, Jan 15, 2016 at 12:22 AM, Kees Cook <keescook@chromium.org> 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@chromium.org> >> >> Looks good. Ack. > > While we're at it: > > int should_remove_suid(struct dentry *dentry) > { > umode_t mode = d_inode(dentry)->i_mode; > int kill = 0; > > /* suid always must be killed */ > if (unlikely(mode & S_ISUID)) > kill = ATTR_KILL_SUID; > > /* > * sgid without any exec bits is just a mandatory locking mark; leave > * it alone. If some exec bits are set, it's a real sgid; kill it. > */ > if (unlikely((mode & S_ISGID) && (mode & S_IXGRP))) > kill |= ATTR_KILL_SGID; > > if (unlikely(kill && !capable(CAP_FSETID) && S_ISREG(mode))) > return kill; > > return 0; > } > EXPORT_SYMBOL(should_remove_suid); > > Oh wait, is that an implicit use of current_cred in vfs_write? No, it > couldn't be. Kernel developers *never* make that mistake. > > This is, of course, totally fucked because this function doesn't have > access to a struct file and therefore can't see f_cred. I'm not going > to look in to this right now, but I swear I saw an exploit that took > advantage of this bug recently. Anyone want to try to fix it? Good point. it's here since 2.3.43. As I see file->f_cred is reachable in all places. > > FWIW, posix says (man 3p write): > > Upon successful completion, where nbyte is greater than 0, write() > shall mark for update the last data modification and last file status > change timestamps of the file, and if the file is a regular file, the > S_ISUID and S_ISGID bits of the file mode may be cleared. > > so maybe the thing to do is just drop the capable check entirely and > cross our fingers that nothing was relying on it. > > --Andy > >> >>> --- >>> 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(¤t->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(¤t->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 > > > > -- > Andy Lutomirski > AMA Capital Management, LLC -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@kvack.org. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a> ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v9] fs: clear file privilege bits when mmap writing 2016-01-15 5:55 ` Konstantin Khlebnikov 2016-01-15 6:18 ` Andy Lutomirski @ 2016-05-25 21:36 ` Kees Cook 2016-05-25 21:49 ` Al Viro 1 sibling, 1 reply; 8+ messages in thread From: Kees Cook @ 2016-05-25 21:36 UTC (permalink / raw) To: Alexander Viro, Andrew Morton Cc: Konstantin Khlebnikov, Andy Lutomirski, Jan Kara, yalin wang, Willy Tarreau, linux-mm@kvack.org, Linux Kernel Mailing List On Thu, Jan 14, 2016 at 9:55 PM, Konstantin Khlebnikov <koct9i@gmail.com> wrote: > On Fri, Jan 15, 2016 at 12:22 AM, Kees Cook <keescook@chromium.org> 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@chromium.org> > > 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(¤t->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(¤t->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 -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@kvack.org. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a> ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v9] fs: clear file privilege bits when mmap writing 2016-05-25 21:36 ` Kees Cook @ 2016-05-25 21:49 ` Al Viro 2017-01-28 3:47 ` Andy Lutomirski 0 siblings, 1 reply; 8+ messages in thread From: Al Viro @ 2016-05-25 21:49 UTC (permalink / raw) To: Kees Cook Cc: Andrew Morton, Konstantin Khlebnikov, Andy Lutomirski, Jan Kara, yalin wang, Willy Tarreau, linux-mm@kvack.org, Linux Kernel Mailing List On Wed, May 25, 2016 at 02:36:57PM -0700, Kees Cook wrote: > 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? ->i_mutex is dealt with just by using lock_inode(inode)/unlock_inode(inode); I hadn't looked at the rest of the locking in there. -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@kvack.org. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a> ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v9] fs: clear file privilege bits when mmap writing 2016-05-25 21:49 ` Al Viro @ 2017-01-28 3:47 ` Andy Lutomirski 2017-02-07 20:18 ` Kees Cook 0 siblings, 1 reply; 8+ messages in thread From: Andy Lutomirski @ 2017-01-28 3:47 UTC (permalink / raw) To: Al Viro Cc: Kees Cook, Andrew Morton, Konstantin Khlebnikov, Jan Kara, yalin wang, Willy Tarreau, linux-mm@kvack.org, Linux Kernel Mailing List On Wed, May 25, 2016 at 2:49 PM, Al Viro <viro@zeniv.linux.org.uk> wrote: > On Wed, May 25, 2016 at 02:36:57PM -0700, Kees Cook wrote: > >> 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? > > ->i_mutex is dealt with just by using lock_inode(inode)/unlock_inode(inode); > I hadn't looked at the rest of the locking in there. Ping? If this is too messy, I'm wondering if we could get away with a much simpler approach: clear the suid and sgid bits when the file is opened for write. -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@kvack.org. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a> ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v9] fs: clear file privilege bits when mmap writing 2017-01-28 3:47 ` Andy Lutomirski @ 2017-02-07 20:18 ` Kees Cook 0 siblings, 0 replies; 8+ messages in thread From: Kees Cook @ 2017-02-07 20:18 UTC (permalink / raw) To: Andy Lutomirski Cc: Al Viro, Andrew Morton, Konstantin Khlebnikov, Jan Kara, yalin wang, Willy Tarreau, linux-mm@kvack.org, Linux Kernel Mailing List On Fri, Jan 27, 2017 at 7:47 PM, Andy Lutomirski <luto@kernel.org> wrote: > On Wed, May 25, 2016 at 2:49 PM, Al Viro <viro@zeniv.linux.org.uk> wrote: >> On Wed, May 25, 2016 at 02:36:57PM -0700, Kees Cook wrote: >> >>> 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? >> >> ->i_mutex is dealt with just by using lock_inode(inode)/unlock_inode(inode); >> I hadn't looked at the rest of the locking in there. > > Ping? > > If this is too messy, I'm wondering if we could get away with a much > simpler approach: clear the suid and sgid bits when the file is opened > for write. I think that'll break something, but I don't have any actual examples. Regardless, I'd still like to see this hole fixed... -Kees -- Kees Cook Pixel Security -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@kvack.org. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a> ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2017-02-07 20:18 UTC | newest] Thread overview: 8+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2016-01-14 21:22 [PATCH v9] fs: clear file privilege bits when mmap writing Kees Cook 2016-01-15 5:55 ` Konstantin Khlebnikov 2016-01-15 6:18 ` Andy Lutomirski 2016-01-15 6:36 ` Konstantin Khlebnikov 2016-05-25 21:36 ` Kees Cook 2016-05-25 21:49 ` Al Viro 2017-01-28 3:47 ` Andy Lutomirski 2017-02-07 20:18 ` Kees Cook
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).