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

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

[Kees Cook]

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). We
could do this during vm_mmap_pgoff, but that would need coverage in
mprotect as well, but to check for MAP_SHARED, we'd need to hold mmap_sem
again. We could clear at open() time, but it's possible things are
accidentally opening with O_RDWR and only reading. Better to clear on
close and error failures (i.e. an improvement over now, which is not
clearing at all).

Instead, detect the need to clear the bits during the page fault, and
actually remove the bits during final fput. Since the file was open for
writing, it wouldn't have been possible to execute it yet.

Signed-off-by: Kees Cook <keescook@chromium.org>
---
I think this is the best we can do; everything else is blocked by mmap_sem.
---
 fs/file_table.c    | 11 +++++++++++
 include/linux/fs.h |  1 +
 mm/memory.c        |  1 +
 3 files changed, 13 insertions(+)

diff --git a/fs/file_table.c b/fs/file_table.c
index ad17e05ebf95..3a7eee76ea90 100644
--- a/fs/file_table.c
+++ b/fs/file_table.c
@@ -191,6 +191,17 @@ static void __fput(struct file *file)
 
 	might_sleep();
 
+	/*
+	 * XXX: While avoiding mmap_sem, we've already been written to.
+	 * We must ignore the return value, since we can't reject the
+	 * write.
+	 */
+	if (unlikely(file->f_remove_privs)) {
+		mutex_lock(&inode->i_mutex);
+		file_remove_privs(file);
+		mutex_unlock(&inode->i_mutex);
+	}
+
 	fsnotify_close(file);
 	/*
 	 * The function eventpoll_release() should be the first called
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 3aa514254161..409bd7047e7e 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -872,6 +872,7 @@ struct file {
 	struct list_head	f_tfile_llink;
 #endif /* #ifdef CONFIG_EPOLL */
 	struct address_space	*f_mapping;
+	bool			f_remove_privs;
 } __attribute__((aligned(4)));	/* lest something weird decides that 2 is OK */
 
 struct file_handle {
diff --git a/mm/memory.c b/mm/memory.c
index c387430f06c3..08a77e0cf65f 100644
--- a/mm/memory.c
+++ b/mm/memory.c
@@ -2036,6 +2036,7 @@ static inline int wp_page_reuse(struct mm_struct *mm,
 
 		if (!page_mkwrite)
 			file_update_time(vma->vm_file);
+		vma->vm_file->f_remove_privs = true;
 	}
 
 	return VM_FAULT_WRITE;
-- 
1.9.1


-- 
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>

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

[Christoph Hellwig]

> 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). We
> could do this during vm_mmap_pgoff, but that would need coverage in
> mprotect as well, but to check for MAP_SHARED, we'd need to hold mmap_sem
> again. We could clear at open() time, but it's possible things are
> accidentally opening with O_RDWR and only reading. Better to clear on
> close and error failures (i.e. an improvement over now, which is not
> clearing at all).
> 
> Instead, detect the need to clear the bits during the page fault, and
> actually remove the bits during final fput. Since the file was open for
> writing, it wouldn't have been possible to execute it yet.


> 
> Signed-off-by: Kees Cook <keescook@chromium.org>
> ---
> I think this is the best we can do; everything else is blocked by mmap_sem.

It should be done at mmap time, before even taking mmap_sem.

Adding a new field for this to strut file isn't really acceptable.

--
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>

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

[Kees Cook]

On Wed, Dec 9, 2015 at 5:21 PM, Christoph Hellwig <hch@infradead.org> wrote:
>> 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). We
>> could do this during vm_mmap_pgoff, but that would need coverage in
>> mprotect as well, but to check for MAP_SHARED, we'd need to hold mmap_sem
>> again. We could clear at open() time, but it's possible things are
>> accidentally opening with O_RDWR and only reading. Better to clear on
>> close and error failures (i.e. an improvement over now, which is not
>> clearing at all).
>>
>> Instead, detect the need to clear the bits during the page fault, and
>> actually remove the bits during final fput. Since the file was open for
>> writing, it wouldn't have been possible to execute it yet.
>
>
>>
>> Signed-off-by: Kees Cook <keescook@chromium.org>
>> ---
>> I think this is the best we can do; everything else is blocked by mmap_sem.
>
> It should be done at mmap time, before even taking mmap_sem.
>
> Adding a new field for this to strut file isn't really acceptable.

I already covered this: there's no way to handle the mprotect case --
checking for MAP_SHARED is under mmap_sem still.

-Kees

-- 
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>

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

[Al Viro]

On Wed, Dec 09, 2015 at 02:51:48PM -0800, Kees Cook wrote:
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index 3aa514254161..409bd7047e7e 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -872,6 +872,7 @@ struct file {
>  	struct list_head	f_tfile_llink;
>  #endif /* #ifdef CONFIG_EPOLL */
>  	struct address_space	*f_mapping;
> +	bool			f_remove_privs;

NAK.  If anything, such things belong in ->f_flags.  _If_ this is worth
doing at all, that is.

--
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>

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

[Willy Tarreau]

Hi Kees,

Why not add a new file flag instead ?

Something like this (editing your patch by hand to illustrate) :

diff --git a/fs/file_table.c b/fs/file_table.c
index ad17e05ebf95..3a7eee76ea90 100644
--- a/fs/file_table.c
+++ b/fs/file_table.c
@@ -191,6 +191,17 @@ static void __fput(struct file *file)
 
 	might_sleep();
 
+	/*
+	 * XXX: While avoiding mmap_sem, we've already been written to.
+	 * We must ignore the return value, since we can't reject the
+	 * write.
+	 */
+	if (unlikely(file->f_flags & FL_DROP_PRIVS)) {
+		mutex_lock(&inode->i_mutex);
+		file_remove_privs(file);
+		mutex_unlock(&inode->i_mutex);
+	}
+
 	fsnotify_close(file);
 	/*
 	 * The function eventpoll_release() should be the first called
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 3aa514254161..409bd7047e7e 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -913,3 +913,4 @@
 #define FL_OFDLCK       1024    /* lock is "owned" by struct file */
 #define FL_LAYOUT       2048    /* outstanding pNFS layout */
+#define FL_DROP_PRIVS   4096    /* lest something weird decides that 2 is OK */
 
diff --git a/mm/memory.c b/mm/memory.c
index c387430f06c3..08a77e0cf65f 100644
--- a/mm/memory.c
+++ b/mm/memory.c
@@ -2036,6 +2036,7 @@ static inline int wp_page_reuse(struct mm_struct *mm,
 
 		if (!page_mkwrite)
 			file_update_time(vma->vm_file);
+		vma->vm_file->f_flags |= FL_DROP_PRIVS;
 	}
 
 	return VM_FAULT_WRITE;

Willy

--
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>

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

[Willy Tarreau]

On Thu, Dec 10, 2015 at 08:06:35AM +0100, Willy Tarreau wrote:
> Hi Kees,
> 
> Why not add a new file flag instead ?
> 
> Something like this (editing your patch by hand to illustrate) :
(...)
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index 3aa514254161..409bd7047e7e 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -913,3 +913,4 @@
>  #define FL_OFDLCK       1024    /* lock is "owned" by struct file */
>  #define FL_LAYOUT       2048    /* outstanding pNFS layout */
> +#define FL_DROP_PRIVS   4096    /* lest something weird decides that 2 is OK */

Crap, these ones are for locks, we need to use O_* instead
But anyway you get the idea, I mean there are probably many spare bits
overthere.

Another option I was thinking about was to change f_mode and detect the
change on close. But I don't know what to compare it against.

Willy

--
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>

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

[Kees Cook]

On Wed, Dec 9, 2015 at 11:06 PM, Willy Tarreau <w@1wt.eu> wrote:
> Hi Kees,
>
> Why not add a new file flag instead ?
>
> Something like this (editing your patch by hand to illustrate) :
>
> diff --git a/fs/file_table.c b/fs/file_table.c
> index ad17e05ebf95..3a7eee76ea90 100644
> --- a/fs/file_table.c
> +++ b/fs/file_table.c
> @@ -191,6 +191,17 @@ static void __fput(struct file *file)
>
>         might_sleep();
>
> +       /*
> +        * XXX: While avoiding mmap_sem, we've already been written to.
> +        * We must ignore the return value, since we can't reject the
> +        * write.
> +        */
> +       if (unlikely(file->f_flags & FL_DROP_PRIVS)) {
> +               mutex_lock(&inode->i_mutex);
> +               file_remove_privs(file);
> +               mutex_unlock(&inode->i_mutex);
> +       }
> +
>         fsnotify_close(file);
>         /*
>          * The function eventpoll_release() should be the first called
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index 3aa514254161..409bd7047e7e 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -913,3 +913,4 @@
>  #define FL_OFDLCK       1024    /* lock is "owned" by struct file */
>  #define FL_LAYOUT       2048    /* outstanding pNFS layout */
> +#define FL_DROP_PRIVS   4096    /* lest something weird decides that 2 is OK */
>
> diff --git a/mm/memory.c b/mm/memory.c
> index c387430f06c3..08a77e0cf65f 100644
> --- a/mm/memory.c
> +++ b/mm/memory.c
> @@ -2036,6 +2036,7 @@ static inline int wp_page_reuse(struct mm_struct *mm,
>
>                 if (!page_mkwrite)
>                         file_update_time(vma->vm_file);
> +               vma->vm_file->f_flags |= FL_DROP_PRIVS;
>         }
>
>         return VM_FAULT_WRITE;
>
> Willy
>

Is f_flags safe to write like this without holding a lock?

-Kees

-- 
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>

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

[Willy Tarreau]

On Thu, Dec 10, 2015 at 10:05:50AM -0800, Kees Cook wrote:
> On Wed, Dec 9, 2015 at 11:06 PM, Willy Tarreau <w@1wt.eu> wrote:
> > Hi Kees,
> >
> > Why not add a new file flag instead ?
> >
> > Something like this (editing your patch by hand to illustrate) :
> >
> > diff --git a/fs/file_table.c b/fs/file_table.c
> > index ad17e05ebf95..3a7eee76ea90 100644
> > --- a/fs/file_table.c
> > +++ b/fs/file_table.c
> > @@ -191,6 +191,17 @@ static void __fput(struct file *file)
> >
> >         might_sleep();
> >
> > +       /*
> > +        * XXX: While avoiding mmap_sem, we've already been written to.
> > +        * We must ignore the return value, since we can't reject the
> > +        * write.
> > +        */
> > +       if (unlikely(file->f_flags & FL_DROP_PRIVS)) {
> > +               mutex_lock(&inode->i_mutex);
> > +               file_remove_privs(file);
> > +               mutex_unlock(&inode->i_mutex);
> > +       }
> > +
> >         fsnotify_close(file);
> >         /*
> >          * The function eventpoll_release() should be the first called
> > diff --git a/include/linux/fs.h b/include/linux/fs.h
> > index 3aa514254161..409bd7047e7e 100644
> > --- a/include/linux/fs.h
> > +++ b/include/linux/fs.h
> > @@ -913,3 +913,4 @@
> >  #define FL_OFDLCK       1024    /* lock is "owned" by struct file */
> >  #define FL_LAYOUT       2048    /* outstanding pNFS layout */
> > +#define FL_DROP_PRIVS   4096    /* lest something weird decides that 2 is OK */
> >
> > diff --git a/mm/memory.c b/mm/memory.c
> > index c387430f06c3..08a77e0cf65f 100644
> > --- a/mm/memory.c
> > +++ b/mm/memory.c
> > @@ -2036,6 +2036,7 @@ static inline int wp_page_reuse(struct mm_struct *mm,
> >
> >                 if (!page_mkwrite)
> >                         file_update_time(vma->vm_file);
> > +               vma->vm_file->f_flags |= FL_DROP_PRIVS;
> >         }
> >
> >         return VM_FAULT_WRITE;
> >
> > Willy
> >
> 
> Is f_flags safe to write like this without holding a lock?

Unfortunately I have no idea. I've seen places where it's written without
taking a lock such as in blkdev_open() and I don't think that this one is
called with a lock held.

The comment in fs.h says that spinlock f_lock is here to protect f_flags
(among others) and that it must not be taken from IRQ context. Thus I'd
think we "just" have to take it to remain safe. That would be just one
spinlock per first write via mmap() to a file, I don't know if that's
reasonable or not :-/

Willy

--
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>

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

[Kees Cook]

On Thu, Dec 10, 2015 at 10:16 AM, Willy Tarreau <w@1wt.eu> wrote:
> On Thu, Dec 10, 2015 at 10:05:50AM -0800, Kees Cook wrote:
>> On Wed, Dec 9, 2015 at 11:06 PM, Willy Tarreau <w@1wt.eu> wrote:
>> > Hi Kees,
>> >
>> > Why not add a new file flag instead ?
>> >
>> > Something like this (editing your patch by hand to illustrate) :
>> >
>> > diff --git a/fs/file_table.c b/fs/file_table.c
>> > index ad17e05ebf95..3a7eee76ea90 100644
>> > --- a/fs/file_table.c
>> > +++ b/fs/file_table.c
>> > @@ -191,6 +191,17 @@ static void __fput(struct file *file)
>> >
>> >         might_sleep();
>> >
>> > +       /*
>> > +        * XXX: While avoiding mmap_sem, we've already been written to.
>> > +        * We must ignore the return value, since we can't reject the
>> > +        * write.
>> > +        */
>> > +       if (unlikely(file->f_flags & FL_DROP_PRIVS)) {
>> > +               mutex_lock(&inode->i_mutex);
>> > +               file_remove_privs(file);
>> > +               mutex_unlock(&inode->i_mutex);
>> > +       }
>> > +
>> >         fsnotify_close(file);
>> >         /*
>> >          * The function eventpoll_release() should be the first called
>> > diff --git a/include/linux/fs.h b/include/linux/fs.h
>> > index 3aa514254161..409bd7047e7e 100644
>> > --- a/include/linux/fs.h
>> > +++ b/include/linux/fs.h
>> > @@ -913,3 +913,4 @@
>> >  #define FL_OFDLCK       1024    /* lock is "owned" by struct file */
>> >  #define FL_LAYOUT       2048    /* outstanding pNFS layout */
>> > +#define FL_DROP_PRIVS   4096    /* lest something weird decides that 2 is OK */
>> >
>> > diff --git a/mm/memory.c b/mm/memory.c
>> > index c387430f06c3..08a77e0cf65f 100644
>> > --- a/mm/memory.c
>> > +++ b/mm/memory.c
>> > @@ -2036,6 +2036,7 @@ static inline int wp_page_reuse(struct mm_struct *mm,
>> >
>> >                 if (!page_mkwrite)
>> >                         file_update_time(vma->vm_file);
>> > +               vma->vm_file->f_flags |= FL_DROP_PRIVS;
>> >         }
>> >
>> >         return VM_FAULT_WRITE;
>> >
>> > Willy
>> >
>>
>> Is f_flags safe to write like this without holding a lock?
>
> Unfortunately I have no idea. I've seen places where it's written without
> taking a lock such as in blkdev_open() and I don't think that this one is
> called with a lock held.
>
> The comment in fs.h says that spinlock f_lock is here to protect f_flags
> (among others) and that it must not be taken from IRQ context. Thus I'd
> think we "just" have to take it to remain safe. That would be just one
> spinlock per first write via mmap() to a file, I don't know if that's
> reasonable or not :-/

Al, what's the best way forward here? I created a separate flag
variable so it could be used effectively write-only, with the read
happening only at final fput.

-Kees

-- 
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>

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

[Al Viro]

On Thu, Dec 10, 2015 at 07:16:11PM +0100, Willy Tarreau wrote:

> > Is f_flags safe to write like this without holding a lock?
> 
> Unfortunately I have no idea. I've seen places where it's written without
> taking a lock such as in blkdev_open() and I don't think that this one is
> called with a lock held.

In any ->open() we obviously have nobody else able to find that struct file,
let alone modify it, so there the damn thing is essentially caller-private
and no locking is needed.

--
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>

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

[Kees Cook]

On Thu, Dec 10, 2015 at 11:33 AM, Al Viro <viro@zeniv.linux.org.uk> wrote:
> On Thu, Dec 10, 2015 at 07:16:11PM +0100, Willy Tarreau wrote:
>
>> > Is f_flags safe to write like this without holding a lock?
>>
>> Unfortunately I have no idea. I've seen places where it's written without
>> taking a lock such as in blkdev_open() and I don't think that this one is
>> called with a lock held.
>
> In any ->open() we obviously have nobody else able to find that struct file,
> let alone modify it, so there the damn thing is essentially caller-private
> and no locking is needed.

In open, sure, but what about under mm/memory.c where we're trying to
twiddle it from vma->file->f_flags as in my patch? That seemed like it
would want atomic safety.

-Kees

-- 
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>

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

[Al Viro]

On Thu, Dec 10, 2015 at 11:47:18AM -0800, Kees Cook wrote:

> In open, sure, but what about under mm/memory.c where we're trying to
> twiddle it from vma->file->f_flags as in my patch? That seemed like it
> would want atomic safety.

Sigh...  Again, I'm not at all convinced that this is the right approach,
but generally you need ->f_lock.  And in situations where the bit can
go only off->on, check it lockless, skip the whole thing entirely if it's
already set and grab the spinlock otherwise.

--
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>

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

[Kees Cook]

On Thu, Dec 10, 2015 at 12:27 PM, Al Viro <viro@zeniv.linux.org.uk> wrote:
> On Thu, Dec 10, 2015 at 11:47:18AM -0800, Kees Cook wrote:
>
>> In open, sure, but what about under mm/memory.c where we're trying to
>> twiddle it from vma->file->f_flags as in my patch? That seemed like it
>> would want atomic safety.
>
> Sigh...  Again, I'm not at all convinced that this is the right approach,

I'm open to any suggestions. Every path I've tried has been ultimately
blocked by mmap_sem. :(

> but generally you need ->f_lock.  And in situations where the bit can
> go only off->on, check it lockless, skip the whole thing entirely if it's
> already set and grab the spinlock otherwise.

And I can take f_lock safely under mmap_sem?

-Kees

-- 
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>

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

[Al Viro]

On Thu, Dec 10, 2015 at 01:45:09PM -0800, Kees Cook wrote:
> > but generally you need ->f_lock.  And in situations where the bit can
> > go only off->on, check it lockless, skip the whole thing entirely if it's
> > already set and grab the spinlock otherwise.
> 
> And I can take f_lock safely under mmap_sem?

Are you asking whether it's safe to take a spinlock under an rwsem?

--
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>

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

[Kees Cook]

On Thu, Dec 10, 2015 at 1:56 PM, Al Viro <viro@zeniv.linux.org.uk> wrote:
> On Thu, Dec 10, 2015 at 01:45:09PM -0800, Kees Cook wrote:
>> > but generally you need ->f_lock.  And in situations where the bit can
>> > go only off->on, check it lockless, skip the whole thing entirely if it's
>> > already set and grab the spinlock otherwise.
>>
>> And I can take f_lock safely under mmap_sem?
>
> Are you asking whether it's safe to take a spinlock under an rwsem?

I keep getting various surprises while trying to implement this
change, so yeah, I just want to make sure I won't waste my time adding
taking the spinlock to the patch.

-Kees

-- 
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>

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

[Kees Cook]

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). We
could do this during vm_mmap_pgoff, but that would need coverage in
mprotect as well, but to check for MAP_SHARED, we'd need to hold mmap_sem
again. We could clear at open() time, but it's possible things are
accidentally opening with O_RDWR and only reading. Better to clear on
close and error failures (i.e. an improvement over now, which is not
clearing at all).

Instead, detect the need to clear the bits during the page fault, and
actually remove the bits during final fput. Since the file was open for
writing, it wouldn't have been possible to execute it yet.

Signed-off-by: Kees Cook <keescook@chromium.org>
---
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
---
 fs/file_table.c                  | 11 +++++++++++
 fs/open.c                        |  2 +-
 include/uapi/asm-generic/fcntl.h |  4 ++++
 mm/memory.c                      |  5 +++++
 4 files changed, 21 insertions(+), 1 deletion(-)

diff --git a/fs/file_table.c b/fs/file_table.c
index ad17e05ebf95..4a8b0b4553e9 100644
--- a/fs/file_table.c
+++ b/fs/file_table.c
@@ -191,6 +191,17 @@ static void __fput(struct file *file)
 
 	might_sleep();
 
+	/*
+	 * XXX: While avoiding mmap_sem, we've already been written to.
+	 * We must ignore the return value, since we can't reject the
+	 * write.
+	 */
+	if (unlikely(file->f_flags & O_REMOVEPRIV)) {
+		mutex_lock(&inode->i_mutex);
+		file_remove_privs(file);
+		mutex_unlock(&inode->i_mutex);
+	}
+
 	fsnotify_close(file);
 	/*
 	 * The function eventpoll_release() should be the first called
diff --git a/fs/open.c b/fs/open.c
index b6f1e96a7c0b..89069d16ca80 100644
--- a/fs/open.c
+++ b/fs/open.c
@@ -895,7 +895,7 @@ static inline int build_open_flags(int flags, umode_t mode, struct open_flags *o
 		op->mode = 0;
 
 	/* Must never be set by userspace */
-	flags &= ~FMODE_NONOTIFY & ~O_CLOEXEC;
+	flags &= ~FMODE_NONOTIFY & ~O_CLOEXEC & ~O_REMOVEPRIV;
 
 	/*
 	 * O_SYNC is implemented as __O_SYNC|O_DSYNC.  As many places only
diff --git a/include/uapi/asm-generic/fcntl.h b/include/uapi/asm-generic/fcntl.h
index e063effe0cc1..096c4b3afe6a 100644
--- a/include/uapi/asm-generic/fcntl.h
+++ b/include/uapi/asm-generic/fcntl.h
@@ -88,6 +88,10 @@
 #define __O_TMPFILE	020000000
 #endif
 
+#ifndef O_REMOVEPRIV
+#define O_REMOVEPRIV	040000000
+#endif
+
 /* a horrid kludge trying to make sure that this will fail on old kernels */
 #define O_TMPFILE (__O_TMPFILE | O_DIRECTORY)
 #define O_TMPFILE_MASK (__O_TMPFILE | O_DIRECTORY | O_CREAT)      
diff --git a/mm/memory.c b/mm/memory.c
index c387430f06c3..ad4188a8f279 100644
--- a/mm/memory.c
+++ b/mm/memory.c
@@ -2036,6 +2036,11 @@ static inline int wp_page_reuse(struct mm_struct *mm,
 
 		if (!page_mkwrite)
 			file_update_time(vma->vm_file);
+		if (unlikely((vma->vm_file->f_flags & O_REMOVEPRIV) == 0)) {
+			spin_lock(&vma->vm_file->f_lock);
+			vma->vm_file->f_flags |= O_REMOVEPRIV;
+			spin_unlock(&vma->vm_file->f_lock);
+		}
 	}
 
 	return VM_FAULT_WRITE;
-- 
2.6.3


-- 
Kees Cook
Chrome OS & Brillo Security

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

[Kees Cook]

On Thu, Dec 10, 2015 at 2:33 PM, 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). We
> could do this during vm_mmap_pgoff, but that would need coverage in
> mprotect as well, but to check for MAP_SHARED, we'd need to hold mmap_sem
> again. We could clear at open() time, but it's possible things are
> accidentally opening with O_RDWR and only reading. Better to clear on
> close and error failures (i.e. an improvement over now, which is not
> clearing at all).
>
> Instead, detect the need to clear the bits during the page fault, and
> actually remove the bits during final fput. Since the file was open for
> writing, it wouldn't have been possible to execute it yet.
>
> Signed-off-by: Kees Cook <keescook@chromium.org>

Al, how does this look?

-Kees

> ---
> 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
> ---
>  fs/file_table.c                  | 11 +++++++++++
>  fs/open.c                        |  2 +-
>  include/uapi/asm-generic/fcntl.h |  4 ++++
>  mm/memory.c                      |  5 +++++
>  4 files changed, 21 insertions(+), 1 deletion(-)
>
> diff --git a/fs/file_table.c b/fs/file_table.c
> index ad17e05ebf95..4a8b0b4553e9 100644
> --- a/fs/file_table.c
> +++ b/fs/file_table.c
> @@ -191,6 +191,17 @@ static void __fput(struct file *file)
>
>         might_sleep();
>
> +       /*
> +        * XXX: While avoiding mmap_sem, we've already been written to.
> +        * We must ignore the return value, since we can't reject the
> +        * write.
> +        */
> +       if (unlikely(file->f_flags & O_REMOVEPRIV)) {
> +               mutex_lock(&inode->i_mutex);
> +               file_remove_privs(file);
> +               mutex_unlock(&inode->i_mutex);
> +       }
> +
>         fsnotify_close(file);
>         /*
>          * The function eventpoll_release() should be the first called
> diff --git a/fs/open.c b/fs/open.c
> index b6f1e96a7c0b..89069d16ca80 100644
> --- a/fs/open.c
> +++ b/fs/open.c
> @@ -895,7 +895,7 @@ static inline int build_open_flags(int flags, umode_t mode, struct open_flags *o
>                 op->mode = 0;
>
>         /* Must never be set by userspace */
> -       flags &= ~FMODE_NONOTIFY & ~O_CLOEXEC;
> +       flags &= ~FMODE_NONOTIFY & ~O_CLOEXEC & ~O_REMOVEPRIV;
>
>         /*
>          * O_SYNC is implemented as __O_SYNC|O_DSYNC.  As many places only
> diff --git a/include/uapi/asm-generic/fcntl.h b/include/uapi/asm-generic/fcntl.h
> index e063effe0cc1..096c4b3afe6a 100644
> --- a/include/uapi/asm-generic/fcntl.h
> +++ b/include/uapi/asm-generic/fcntl.h
> @@ -88,6 +88,10 @@
>  #define __O_TMPFILE    020000000
>  #endif
>
> +#ifndef O_REMOVEPRIV
> +#define O_REMOVEPRIV   040000000
> +#endif
> +
>  /* a horrid kludge trying to make sure that this will fail on old kernels */
>  #define O_TMPFILE (__O_TMPFILE | O_DIRECTORY)
>  #define O_TMPFILE_MASK (__O_TMPFILE | O_DIRECTORY | O_CREAT)
> diff --git a/mm/memory.c b/mm/memory.c
> index c387430f06c3..ad4188a8f279 100644
> --- a/mm/memory.c
> +++ b/mm/memory.c
> @@ -2036,6 +2036,11 @@ static inline int wp_page_reuse(struct mm_struct *mm,
>
>                 if (!page_mkwrite)
>                         file_update_time(vma->vm_file);
> +               if (unlikely((vma->vm_file->f_flags & O_REMOVEPRIV) == 0)) {
> +                       spin_lock(&vma->vm_file->f_lock);
> +                       vma->vm_file->f_flags |= O_REMOVEPRIV;
> +                       spin_unlock(&vma->vm_file->f_lock);
> +               }
>         }
>
>         return VM_FAULT_WRITE;
> --
> 2.6.3
>
>
> --
> Kees Cook
> Chrome OS & Brillo Security



-- 
Kees Cook
Chrome OS & Brillo Security

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

[Andy Lutomirski]

On Thu, Dec 10, 2015 at 2:33 PM, Kees Cook <keescook@chromium.org> wrote:
> Instead, detect the need to clear the bits during the page fault, and
> actually remove the bits during final fput. Since the file was open for
> writing, it wouldn't have been possible to execute it yet.

This is cute but mysterious.  Could you add a comment?

>
> +       /*
> +        * XXX: While avoiding mmap_sem, we've already been written to.
> +        * We must ignore the return value, since we can't reject the
> +        * write.
> +        */

e.g. here?