* Re: [PATCH v3 resend 1/2] mm: Add an F_SEAL_FUTURE_WRITE seal to memfd
From: Michael Tirado @ 2018-11-09 20:02 UTC (permalink / raw)
To: Andy Lutomirski
Cc: Jann Horn, joel, LKML, jreck, john.stultz, tkjos, gregkh, hch,
viro, Andrew Morton, dancol, bfields, jlayton, khalid.aziz,
Lei.Yang, linux-fsdevel, linux-kselftest, linux-mm,
marcandre.lureau, mike.kravetz, minchan, shuah, valdis.kletnieks,
hughd, linux-api
In-Reply-To: <A7EC46BC-441A-4A06-9E2F-A26DA88B5320@amacapital.net>
On Fri, Nov 9, 2018 at 9:41 PM Andy Lutomirski <luto@amacapital.net> wrote:
>
>
>
> > On Nov 9, 2018, at 1:06 PM, Jann Horn <jannh@google.com> wrote:
> >
> > +linux-api for API addition
> > +hughd as FYI since this is somewhat related to mm/shmem
> >
> > On Fri, Nov 9, 2018 at 9:46 PM Joel Fernandes (Google)
> > <joel@joelfernandes.org> wrote:
> >> Android uses ashmem for sharing memory regions. We are looking forward
> >> to migrating all usecases of ashmem to memfd so that we can possibly
> >> remove the ashmem driver in the future from staging while also
> >> benefiting from using memfd and contributing to it. Note staging drivers
> >> are also not ABI and generally can be removed at anytime.
> >>
> >> One of the main usecases Android has is the ability to create a region
> >> and mmap it as writeable, then add protection against making any
> >> "future" writes while keeping the existing already mmap'ed
> >> writeable-region active. This allows us to implement a usecase where
> >> receivers of the shared memory buffer can get a read-only view, while
> >> the sender continues to write to the buffer.
Oh I remember trying this years ago with a new seal, F_SEAL_WRITE_PEER,
or something like that.
> >
> > So you're fiddling around with the file, but not the inode? How are
> > you preventing code like the following from re-opening the file as
> > writable?
> >
> > $ cat memfd.c
> > #define _GNU_SOURCE
> > #include <unistd.h>
> > #include <sys/syscall.h>
> > #include <printf.h>
> > #include <fcntl.h>
> > #include <err.h>
> > #include <stdio.h>
> >
> > int main(void) {
> > int fd = syscall(__NR_memfd_create, "testfd", 0);
> > if (fd == -1) err(1, "memfd");
> > char path[100];
> > sprintf(path, "/proc/self/fd/%d", fd);
> > int fd2 = open(path, O_RDWR);
> > if (fd2 == -1) err(1, "reopen");
> > printf("reopen successful: %d\n", fd2);
> > }
> > $ gcc -o memfd memfd.c
> > $ ./memfd
> > reopen successful: 4
> > $
> >
The race condition between memfd_create and applying seals in fcntl?
I think it would be possible to block new write mappings from peer processes if
there is a new memfd_create api that accepts seals. Allowing caller to
set a seal
like the one I proposed years ago, though in a race-free manner. Then
also consider
how to properly handle blocking inherited +W mapping through
clone/fork. Maybe I'm
forgetting some other pitfalls?
> > That aside: I wonder whether a better API would be something that
> > allows you to create a new readonly file descriptor, instead of
> > fiddling with the writability of an existing fd.
>
> Every now and then I try to write a patch to prevent using proc to reopen a file with greater permission than the original open.
>
> I like your idea to have a clean way to reopen a a memfd with reduced permissions. But I would make it a syscall instead and maybe make it only work for memfd at first. And the proc issue would need to be fixed, too.
IMO the best solution would handle the issue at memfd creation time by
removing the race condition.
^ permalink raw reply
* Re: [PATCH v3 resend 1/2] mm: Add an F_SEAL_FUTURE_WRITE seal to memfd
From: Jann Horn @ 2018-11-09 21:06 UTC (permalink / raw)
To: joel
Cc: kernel list, jreck, John Stultz, Todd Kjos, Greg Kroah-Hartman,
Christoph Hellwig, Al Viro, Andrew Morton, Daniel Colascione,
Bruce Fields, jlayton, Khalid Aziz, Lei.Yang, linux-fsdevel,
linux-kselftest, Linux-MM, marcandre.lureau, Mike Kravetz,
minchan, shuah, valdis.kletnieks, Hugh Dickins, Linux API
In-Reply-To: <20181108041537.39694-1-joel@joelfernandes.org>
+linux-api for API addition
+hughd as FYI since this is somewhat related to mm/shmem
On Fri, Nov 9, 2018 at 9:46 PM Joel Fernandes (Google)
<joel@joelfernandes.org> wrote:
> Android uses ashmem for sharing memory regions. We are looking forward
> to migrating all usecases of ashmem to memfd so that we can possibly
> remove the ashmem driver in the future from staging while also
> benefiting from using memfd and contributing to it. Note staging drivers
> are also not ABI and generally can be removed at anytime.
>
> One of the main usecases Android has is the ability to create a region
> and mmap it as writeable, then add protection against making any
> "future" writes while keeping the existing already mmap'ed
> writeable-region active. This allows us to implement a usecase where
> receivers of the shared memory buffer can get a read-only view, while
> the sender continues to write to the buffer.
> See CursorWindow documentation in Android for more details:
> https://developer.android.com/reference/android/database/CursorWindow
>
> This usecase cannot be implemented with the existing F_SEAL_WRITE seal.
> To support the usecase, this patch adds a new F_SEAL_FUTURE_WRITE seal
> which prevents any future mmap and write syscalls from succeeding while
> keeping the existing mmap active.
Please CC linux-api@ on patches like this. If you had done that, I
might have criticized your v1 patch instead of your v3 patch...
> The following program shows the seal
> working in action:
[...]
> Cc: jreck@google.com
> Cc: john.stultz@linaro.org
> Cc: tkjos@google.com
> Cc: gregkh@linuxfoundation.org
> Cc: hch@infradead.org
> Reviewed-by: John Stultz <john.stultz@linaro.org>
> Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
> ---
[...]
> diff --git a/mm/memfd.c b/mm/memfd.c
> index 2bb5e257080e..5ba9804e9515 100644
> --- a/mm/memfd.c
> +++ b/mm/memfd.c
[...]
> @@ -219,6 +220,25 @@ static int memfd_add_seals(struct file *file, unsigned int seals)
> }
> }
>
> + if ((seals & F_SEAL_FUTURE_WRITE) &&
> + !(*file_seals & F_SEAL_FUTURE_WRITE)) {
> + /*
> + * The FUTURE_WRITE seal also prevents growing and shrinking
> + * so we need them to be already set, or requested now.
> + */
> + int test_seals = (seals | *file_seals) &
> + (F_SEAL_GROW | F_SEAL_SHRINK);
> +
> + if (test_seals != (F_SEAL_GROW | F_SEAL_SHRINK)) {
> + error = -EINVAL;
> + goto unlock;
> + }
> +
> + spin_lock(&file->f_lock);
> + file->f_mode &= ~(FMODE_WRITE | FMODE_PWRITE);
> + spin_unlock(&file->f_lock);
> + }
So you're fiddling around with the file, but not the inode? How are
you preventing code like the following from re-opening the file as
writable?
$ cat memfd.c
#define _GNU_SOURCE
#include <unistd.h>
#include <sys/syscall.h>
#include <printf.h>
#include <fcntl.h>
#include <err.h>
#include <stdio.h>
int main(void) {
int fd = syscall(__NR_memfd_create, "testfd", 0);
if (fd == -1) err(1, "memfd");
char path[100];
sprintf(path, "/proc/self/fd/%d", fd);
int fd2 = open(path, O_RDWR);
if (fd2 == -1) err(1, "reopen");
printf("reopen successful: %d\n", fd2);
}
$ gcc -o memfd memfd.c
$ ./memfd
reopen successful: 4
$
That aside: I wonder whether a better API would be something that
allows you to create a new readonly file descriptor, instead of
fiddling with the writability of an existing fd.
^ permalink raw reply
* Re: [PATCH v3 resend 1/2] mm: Add an F_SEAL_FUTURE_WRITE seal to memfd
From: Jann Horn @ 2018-11-09 21:19 UTC (permalink / raw)
To: joel
Cc: kernel list, jreck, John Stultz, Todd Kjos, Greg Kroah-Hartman,
Christoph Hellwig, Al Viro, Andrew Morton, Daniel Colascione,
Bruce Fields, jlayton, Khalid Aziz, Lei.Yang, linux-fsdevel,
linux-kselftest, Linux-MM, marcandre.lureau, Mike Kravetz,
minchan, shuah, valdis.kletnieks, Hugh Dickins, Linux API
In-Reply-To: <CAG48ez1h=v-JYnDw81HaYJzOfrNhwYksxmc2r=cJvdQVgYM+NA@mail.gmail.com>
On Fri, Nov 9, 2018 at 10:06 PM Jann Horn <jannh@google.com> wrote:
> On Fri, Nov 9, 2018 at 9:46 PM Joel Fernandes (Google)
> <joel@joelfernandes.org> wrote:
> > Android uses ashmem for sharing memory regions. We are looking forward
> > to migrating all usecases of ashmem to memfd so that we can possibly
> > remove the ashmem driver in the future from staging while also
> > benefiting from using memfd and contributing to it. Note staging drivers
> > are also not ABI and generally can be removed at anytime.
> >
> > One of the main usecases Android has is the ability to create a region
> > and mmap it as writeable, then add protection against making any
> > "future" writes while keeping the existing already mmap'ed
> > writeable-region active. This allows us to implement a usecase where
> > receivers of the shared memory buffer can get a read-only view, while
> > the sender continues to write to the buffer.
> > See CursorWindow documentation in Android for more details:
> > https://developer.android.com/reference/android/database/CursorWindow
> >
> > This usecase cannot be implemented with the existing F_SEAL_WRITE seal.
> > To support the usecase, this patch adds a new F_SEAL_FUTURE_WRITE seal
> > which prevents any future mmap and write syscalls from succeeding while
> > keeping the existing mmap active.
>
> Please CC linux-api@ on patches like this. If you had done that, I
> might have criticized your v1 patch instead of your v3 patch...
>
> > The following program shows the seal
> > working in action:
> [...]
> > Cc: jreck@google.com
> > Cc: john.stultz@linaro.org
> > Cc: tkjos@google.com
> > Cc: gregkh@linuxfoundation.org
> > Cc: hch@infradead.org
> > Reviewed-by: John Stultz <john.stultz@linaro.org>
> > Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
> > ---
> [...]
> > diff --git a/mm/memfd.c b/mm/memfd.c
> > index 2bb5e257080e..5ba9804e9515 100644
> > --- a/mm/memfd.c
> > +++ b/mm/memfd.c
> [...]
> > @@ -219,6 +220,25 @@ static int memfd_add_seals(struct file *file, unsigned int seals)
> > }
> > }
> >
> > + if ((seals & F_SEAL_FUTURE_WRITE) &&
> > + !(*file_seals & F_SEAL_FUTURE_WRITE)) {
> > + /*
> > + * The FUTURE_WRITE seal also prevents growing and shrinking
> > + * so we need them to be already set, or requested now.
> > + */
> > + int test_seals = (seals | *file_seals) &
> > + (F_SEAL_GROW | F_SEAL_SHRINK);
> > +
> > + if (test_seals != (F_SEAL_GROW | F_SEAL_SHRINK)) {
> > + error = -EINVAL;
> > + goto unlock;
> > + }
> > +
> > + spin_lock(&file->f_lock);
> > + file->f_mode &= ~(FMODE_WRITE | FMODE_PWRITE);
> > + spin_unlock(&file->f_lock);
> > + }
>
> So you're fiddling around with the file, but not the inode? How are
> you preventing code like the following from re-opening the file as
> writable?
>
> $ cat memfd.c
> #define _GNU_SOURCE
> #include <unistd.h>
> #include <sys/syscall.h>
> #include <printf.h>
> #include <fcntl.h>
> #include <err.h>
> #include <stdio.h>
>
> int main(void) {
> int fd = syscall(__NR_memfd_create, "testfd", 0);
> if (fd == -1) err(1, "memfd");
> char path[100];
> sprintf(path, "/proc/self/fd/%d", fd);
> int fd2 = open(path, O_RDWR);
> if (fd2 == -1) err(1, "reopen");
> printf("reopen successful: %d\n", fd2);
> }
> $ gcc -o memfd memfd.c
> $ ./memfd
> reopen successful: 4
> $
>
> That aside: I wonder whether a better API would be something that
> allows you to create a new readonly file descriptor, instead of
> fiddling with the writability of an existing fd.
My favorite approach would be to forbid open() on memfds, hope that
nobody notices the tiny API break, and then add an ioctl for "reopen
this memfd with reduced permissions" - but that's just my personal
opinion.
^ permalink raw reply
* Re: [PATCH v3 resend 1/2] mm: Add an F_SEAL_FUTURE_WRITE seal to memfd
From: Andy Lutomirski @ 2018-11-09 21:40 UTC (permalink / raw)
To: Jann Horn
Cc: joel, kernel list, jreck, John Stultz, Todd Kjos,
Greg Kroah-Hartman, Christoph Hellwig, Al Viro, Andrew Morton,
Daniel Colascione, Bruce Fields, jlayton, Khalid Aziz, Lei.Yang,
linux-fsdevel, linux-kselftest, Linux-MM, marcandre.lureau,
Mike Kravetz, minchan, shuah, valdis.kletnieks, Hugh Dickins,
Linux API
In-Reply-To: <CAG48ez1h=v-JYnDw81HaYJzOfrNhwYksxmc2r=cJvdQVgYM+NA@mail.gmail.com>
> On Nov 9, 2018, at 1:06 PM, Jann Horn <jannh@google.com> wrote:
>
> +linux-api for API addition
> +hughd as FYI since this is somewhat related to mm/shmem
>
> On Fri, Nov 9, 2018 at 9:46 PM Joel Fernandes (Google)
> <joel@joelfernandes.org> wrote:
>> Android uses ashmem for sharing memory regions. We are looking forward
>> to migrating all usecases of ashmem to memfd so that we can possibly
>> remove the ashmem driver in the future from staging while also
>> benefiting from using memfd and contributing to it. Note staging drivers
>> are also not ABI and generally can be removed at anytime.
>>
>> One of the main usecases Android has is the ability to create a region
>> and mmap it as writeable, then add protection against making any
>> "future" writes while keeping the existing already mmap'ed
>> writeable-region active. This allows us to implement a usecase where
>> receivers of the shared memory buffer can get a read-only view, while
>> the sender continues to write to the buffer.
>> See CursorWindow documentation in Android for more details:
>> https://developer.android.com/reference/android/database/CursorWindow
>>
>> This usecase cannot be implemented with the existing F_SEAL_WRITE seal.
>> To support the usecase, this patch adds a new F_SEAL_FUTURE_WRITE seal
>> which prevents any future mmap and write syscalls from succeeding while
>> keeping the existing mmap active.
>
> Please CC linux-api@ on patches like this. If you had done that, I
> might have criticized your v1 patch instead of your v3 patch...
>
>> The following program shows the seal
>> working in action:
> [...]
>> Cc: jreck@google.com
>> Cc: john.stultz@linaro.org
>> Cc: tkjos@google.com
>> Cc: gregkh@linuxfoundation.org
>> Cc: hch@infradead.org
>> Reviewed-by: John Stultz <john.stultz@linaro.org>
>> Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
>> ---
> [...]
>> diff --git a/mm/memfd.c b/mm/memfd.c
>> index 2bb5e257080e..5ba9804e9515 100644
>> --- a/mm/memfd.c
>> +++ b/mm/memfd.c
> [...]
>> @@ -219,6 +220,25 @@ static int memfd_add_seals(struct file *file, unsigned int seals)
>> }
>> }
>>
>> + if ((seals & F_SEAL_FUTURE_WRITE) &&
>> + !(*file_seals & F_SEAL_FUTURE_WRITE)) {
>> + /*
>> + * The FUTURE_WRITE seal also prevents growing and shrinking
>> + * so we need them to be already set, or requested now.
>> + */
>> + int test_seals = (seals | *file_seals) &
>> + (F_SEAL_GROW | F_SEAL_SHRINK);
>> +
>> + if (test_seals != (F_SEAL_GROW | F_SEAL_SHRINK)) {
>> + error = -EINVAL;
>> + goto unlock;
>> + }
>> +
>> + spin_lock(&file->f_lock);
>> + file->f_mode &= ~(FMODE_WRITE | FMODE_PWRITE);
>> + spin_unlock(&file->f_lock);
>> + }
>
> So you're fiddling around with the file, but not the inode? How are
> you preventing code like the following from re-opening the file as
> writable?
>
> $ cat memfd.c
> #define _GNU_SOURCE
> #include <unistd.h>
> #include <sys/syscall.h>
> #include <printf.h>
> #include <fcntl.h>
> #include <err.h>
> #include <stdio.h>
>
> int main(void) {
> int fd = syscall(__NR_memfd_create, "testfd", 0);
> if (fd == -1) err(1, "memfd");
> char path[100];
> sprintf(path, "/proc/self/fd/%d", fd);
> int fd2 = open(path, O_RDWR);
> if (fd2 == -1) err(1, "reopen");
> printf("reopen successful: %d\n", fd2);
> }
> $ gcc -o memfd memfd.c
> $ ./memfd
> reopen successful: 4
> $
>
> That aside: I wonder whether a better API would be something that
> allows you to create a new readonly file descriptor, instead of
> fiddling with the writability of an existing fd.
Every now and then I try to write a patch to prevent using proc to reopen a file with greater permission than the original open.
I like your idea to have a clean way to reopen a a memfd with reduced permissions. But I would make it a syscall instead and maybe make it only work for memfd at first. And the proc issue would need to be fixed, too.
^ permalink raw reply
* Re: [PATCH v3 resend 1/2] mm: Add an F_SEAL_FUTURE_WRITE seal to memfd
From: Daniel Colascione @ 2018-11-09 22:20 UTC (permalink / raw)
To: Jann Horn
Cc: Joel Fernandes, kernel list, John Reck, John Stultz, Todd Kjos,
Greg Kroah-Hartman, Christoph Hellwig, Al Viro, Andrew Morton,
Bruce Fields, Jeff Layton, Khalid Aziz, Lei.Yang, linux-fsdevel,
linux-kselftest, Linux-MM, marcandre.lureau, Mike Kravetz,
Minchan Kim, Shuah Khan, valdis.kletnieks, Hugh Dickins
In-Reply-To: <CAG48ez1h=v-JYnDw81HaYJzOfrNhwYksxmc2r=cJvdQVgYM+NA@mail.gmail.com>
On Fri, Nov 9, 2018 at 1:06 PM, Jann Horn <jannh@google.com> wrote:
>
> +linux-api for API addition
> +hughd as FYI since this is somewhat related to mm/shmem
>
> On Fri, Nov 9, 2018 at 9:46 PM Joel Fernandes (Google)
> <joel@joelfernandes.org> wrote:
> > Android uses ashmem for sharing memory regions. We are looking forward
> > to migrating all usecases of ashmem to memfd so that we can possibly
> > remove the ashmem driver in the future from staging while also
> > benefiting from using memfd and contributing to it. Note staging drivers
> > are also not ABI and generally can be removed at anytime.
> >
> > One of the main usecases Android has is the ability to create a region
> > and mmap it as writeable, then add protection against making any
> > "future" writes while keeping the existing already mmap'ed
> > writeable-region active. This allows us to implement a usecase where
> > receivers of the shared memory buffer can get a read-only view, while
> > the sender continues to write to the buffer.
> > See CursorWindow documentation in Android for more details:
> > https://developer.android.com/reference/android/database/CursorWindow
> >
> > This usecase cannot be implemented with the existing F_SEAL_WRITE seal.
> > To support the usecase, this patch adds a new F_SEAL_FUTURE_WRITE seal
> > which prevents any future mmap and write syscalls from succeeding while
> > keeping the existing mmap active.
>
> Please CC linux-api@ on patches like this. If you had done that, I
> might have criticized your v1 patch instead of your v3 patch...
>
> > The following program shows the seal
> > working in action:
> [...]
> > Cc: jreck@google.com
> > Cc: john.stultz@linaro.org
> > Cc: tkjos@google.com
> > Cc: gregkh@linuxfoundation.org
> > Cc: hch@infradead.org
> > Reviewed-by: John Stultz <john.stultz@linaro.org>
> > Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
> > ---
> [...]
> > diff --git a/mm/memfd.c b/mm/memfd.c
> > index 2bb5e257080e..5ba9804e9515 100644
> > --- a/mm/memfd.c
> > +++ b/mm/memfd.c
> [...]
> > @@ -219,6 +220,25 @@ static int memfd_add_seals(struct file *file, unsigned int seals)
> > }
> > }
> >
> > + if ((seals & F_SEAL_FUTURE_WRITE) &&
> > + !(*file_seals & F_SEAL_FUTURE_WRITE)) {
> > + /*
> > + * The FUTURE_WRITE seal also prevents growing and shrinking
> > + * so we need them to be already set, or requested now.
> > + */
> > + int test_seals = (seals | *file_seals) &
> > + (F_SEAL_GROW | F_SEAL_SHRINK);
> > +
> > + if (test_seals != (F_SEAL_GROW | F_SEAL_SHRINK)) {
> > + error = -EINVAL;
> > + goto unlock;
> > + }
> > +
> > + spin_lock(&file->f_lock);
> > + file->f_mode &= ~(FMODE_WRITE | FMODE_PWRITE);
> > + spin_unlock(&file->f_lock);
> > + }
>
> So you're fiddling around with the file, but not the inode? How are
> you preventing code like the following from re-opening the file as
> writable?
Good catch. That's fixable too though, isn't it, just by fiddling with
the inode, right?
Another, more general fix might be to prevent /proc/pid/fd/N opens
from "upgrading" access modes. But that'd be a bigger ABI break.
> That aside: I wonder whether a better API would be something that
> allows you to create a new readonly file descriptor, instead of
> fiddling with the writability of an existing fd.
That doesn't work, unfortunately. The ashmem API we're replacing with
memfd requires file descriptor continuity. I also looked into opening
a new FD and dup2(2)ing atop the old one, but this approach doesn't
work in the case that the old FD has already leaked to some other
context (e.g., another dup, SCM_RIGHTS). See
https://developer.android.com/ndk/reference/group/memory. We can't
break ASharedMemory_setProt.
^ permalink raw reply
* Re: [PATCH v3 resend 1/2] mm: Add an F_SEAL_FUTURE_WRITE seal to memfd
From: Andy Lutomirski @ 2018-11-09 22:37 UTC (permalink / raw)
To: Daniel Colascione
Cc: Jann Horn, Joel Fernandes, kernel list, John Reck, John Stultz,
Todd Kjos, Greg Kroah-Hartman, Christoph Hellwig, Al Viro,
Andrew Morton, Bruce Fields, Jeff Layton, Khalid Aziz, Lei.Yang,
linux-fsdevel, linux-kselftest, Linux-MM, marcandre.lureau,
Mike Kravetz, Minchan Kim, Shuah Khan, valdis.kletnieks
In-Reply-To: <CAKOZuesw1wG-YynWL7bVb+4BWtYp0Ei62vweWF+mqF1Ln-_2Tg@mail.gmail.com>
> On Nov 9, 2018, at 2:20 PM, Daniel Colascione <dancol@google.com> wrote:
>
>> On Fri, Nov 9, 2018 at 1:06 PM, Jann Horn <jannh@google.com> wrote:
>>
>> +linux-api for API addition
>> +hughd as FYI since this is somewhat related to mm/shmem
>>
>> On Fri, Nov 9, 2018 at 9:46 PM Joel Fernandes (Google)
>> <joel@joelfernandes.org> wrote:
>>> Android uses ashmem for sharing memory regions. We are looking forward
>>> to migrating all usecases of ashmem to memfd so that we can possibly
>>> remove the ashmem driver in the future from staging while also
>>> benefiting from using memfd and contributing to it. Note staging drivers
>>> are also not ABI and generally can be removed at anytime.
>>>
>>> One of the main usecases Android has is the ability to create a region
>>> and mmap it as writeable, then add protection against making any
>>> "future" writes while keeping the existing already mmap'ed
>>> writeable-region active. This allows us to implement a usecase where
>>> receivers of the shared memory buffer can get a read-only view, while
>>> the sender continues to write to the buffer.
>>> See CursorWindow documentation in Android for more details:
>>> https://developer.android.com/reference/android/database/CursorWindow
>>>
>>> This usecase cannot be implemented with the existing F_SEAL_WRITE seal.
>>> To support the usecase, this patch adds a new F_SEAL_FUTURE_WRITE seal
>>> which prevents any future mmap and write syscalls from succeeding while
>>> keeping the existing mmap active.
>>
>> Please CC linux-api@ on patches like this. If you had done that, I
>> might have criticized your v1 patch instead of your v3 patch...
>>
>>> The following program shows the seal
>>> working in action:
>> [...]
>>> Cc: jreck@google.com
>>> Cc: john.stultz@linaro.org
>>> Cc: tkjos@google.com
>>> Cc: gregkh@linuxfoundation.org
>>> Cc: hch@infradead.org
>>> Reviewed-by: John Stultz <john.stultz@linaro.org>
>>> Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
>>> ---
>> [...]
>>> diff --git a/mm/memfd.c b/mm/memfd.c
>>> index 2bb5e257080e..5ba9804e9515 100644
>>> --- a/mm/memfd.c
>>> +++ b/mm/memfd.c
>> [...]
>>> @@ -219,6 +220,25 @@ static int memfd_add_seals(struct file *file, unsigned int seals)
>>> }
>>> }
>>>
>>> + if ((seals & F_SEAL_FUTURE_WRITE) &&
>>> + !(*file_seals & F_SEAL_FUTURE_WRITE)) {
>>> + /*
>>> + * The FUTURE_WRITE seal also prevents growing and shrinking
>>> + * so we need them to be already set, or requested now.
>>> + */
>>> + int test_seals = (seals | *file_seals) &
>>> + (F_SEAL_GROW | F_SEAL_SHRINK);
>>> +
>>> + if (test_seals != (F_SEAL_GROW | F_SEAL_SHRINK)) {
>>> + error = -EINVAL;
>>> + goto unlock;
>>> + }
>>> +
>>> + spin_lock(&file->f_lock);
>>> + file->f_mode &= ~(FMODE_WRITE | FMODE_PWRITE);
>>> + spin_unlock(&file->f_lock);
>>> + }
>>
>> So you're fiddling around with the file, but not the inode? How are
>> you preventing code like the following from re-opening the file as
>> writable?
>
> Good catch. That's fixable too though, isn't it, just by fiddling with
> the inode, right?
True.
>
> Another, more general fix might be to prevent /proc/pid/fd/N opens
> from "upgrading" access modes. But that'd be a bigger ABI break.
I think we should fix that, too. I consider it a bug fix, not an ABI break, personally.
>
>> That aside: I wonder whether a better API would be something that
>> allows you to create a new readonly file descriptor, instead of
>> fiddling with the writability of an existing fd.
>
> That doesn't work, unfortunately. The ashmem API we're replacing with
> memfd requires file descriptor continuity. I also looked into opening
> a new FD and dup2(2)ing atop the old one, but this approach doesn't
> work in the case that the old FD has already leaked to some other
> context (e.g., another dup, SCM_RIGHTS). See
> https://developer.android.com/ndk/reference/group/memory. We can't
> break ASharedMemory_setProt.
Hmm. If we fix the general reopen bug, a way to drop write access from an existing struct file would do what Android needs, right? I don’t know if there are general VFS issues with that.
^ permalink raw reply
* Re: [PATCH v3 resend 1/2] mm: Add an F_SEAL_FUTURE_WRITE seal to memfd
From: Daniel Colascione @ 2018-11-09 22:42 UTC (permalink / raw)
To: Andy Lutomirski
Cc: Jann Horn, Joel Fernandes, kernel list, John Reck, John Stultz,
Todd Kjos, Greg Kroah-Hartman, Christoph Hellwig, Al Viro,
Andrew Morton, Bruce Fields, Jeff Layton, Khalid Aziz, Lei.Yang,
linux-fsdevel, linux-kselftest, Linux-MM, marcandre.lureau,
Mike Kravetz, Minchan Kim, Shuah Khan, valdis.kletnieks
In-Reply-To: <BB64C995-F374-49EB-8469-4820231D8152@amacapital.net>
On Fri, Nov 9, 2018 at 2:37 PM, Andy Lutomirski <luto@amacapital.net> wrote:
>> Another, more general fix might be to prevent /proc/pid/fd/N opens
>> from "upgrading" access modes. But that'd be a bigger ABI break.
>
> I think we should fix that, too. I consider it a bug fix, not an ABI break, personally.
Someone, somewhere is probably relying on it though, and that means
that we probably can't change it unless it's actually causing
problems.
<mumble>spacebar heating</mumble>
>>> That aside: I wonder whether a better API would be something that
>>> allows you to create a new readonly file descriptor, instead of
>>> fiddling with the writability of an existing fd.
>>
>> That doesn't work, unfortunately. The ashmem API we're replacing with
>> memfd requires file descriptor continuity. I also looked into opening
>> a new FD and dup2(2)ing atop the old one, but this approach doesn't
>> work in the case that the old FD has already leaked to some other
>> context (e.g., another dup, SCM_RIGHTS). See
>> https://developer.android.com/ndk/reference/group/memory. We can't
>> break ASharedMemory_setProt.
>
>
> Hmm. If we fix the general reopen bug, a way to drop write access from an existing struct file would do what Android needs, right? I don’t know if there are general VFS issues with that.
I also proposed that. :-) Maybe it'd work best as a special case of
the perennial revoke(2) that people keep proposing. You'd be able to
selectively revoke all access or just write access.
^ permalink raw reply
* Re: [PATCH v3 resend 1/2] mm: Add an F_SEAL_FUTURE_WRITE seal to memfd
From: Andy Lutomirski @ 2018-11-09 23:14 UTC (permalink / raw)
To: Daniel Colascione
Cc: Jann Horn, Joel Fernandes, kernel list, John Reck, John Stultz,
Todd Kjos, Greg Kroah-Hartman, Christoph Hellwig, Al Viro,
Andrew Morton, Bruce Fields, Jeff Layton, Khalid Aziz, Lei.Yang,
linux-fsdevel, linux-kselftest, Linux-MM, marcandre.lureau,
Mike Kravetz, Minchan Kim, Shuah Khan, valdis.kletnieks
In-Reply-To: <CAKOZuetZrL10zWwn4Jzzg0QL2nd3Fm0JxGtzC79SZAfOK525Ag@mail.gmail.com>
> On Nov 9, 2018, at 2:42 PM, Daniel Colascione <dancol@google.com> wrote:
>
> On Fri, Nov 9, 2018 at 2:37 PM, Andy Lutomirski <luto@amacapital.net> wrote:
>>> Another, more general fix might be to prevent /proc/pid/fd/N opens
>>> from "upgrading" access modes. But that'd be a bigger ABI break.
>>
>> I think we should fix that, too. I consider it a bug fix, not an ABI break, personally.
>
> Someone, somewhere is probably relying on it though, and that means
> that we probably can't change it unless it's actually causing
> problems.
>
> <mumble>spacebar heating</mumble>
I think it has caused problems in the past. It’s certainly extremely surprising behavior. I’d say it should be fixed and, if needed, a sysctl to unfix it might be okay.
>
>>>> That aside: I wonder whether a better API would be something that
>>>> allows you to create a new readonly file descriptor, instead of
>>>> fiddling with the writability of an existing fd.
>>>
>>> That doesn't work, unfortunately. The ashmem API we're replacing with
>>> memfd requires file descriptor continuity. I also looked into opening
>>> a new FD and dup2(2)ing atop the old one, but this approach doesn't
>>> work in the case that the old FD has already leaked to some other
>>> context (e.g., another dup, SCM_RIGHTS). See
>>> https://developer.android.com/ndk/reference/group/memory. We can't
>>> break ASharedMemory_setProt.
>>
>>
>> Hmm. If we fix the general reopen bug, a way to drop write access from an existing struct file would do what Android needs, right? I don’t know if there are general VFS issues with that.
>
> I also proposed that. :-) Maybe it'd work best as a special case of
> the perennial revoke(2) that people keep proposing. You'd be able to
> selectively revoke all access or just write access.
Sounds good to me, modulo possible races, but that shouldn’t be too hard to deal with.
^ permalink raw reply
* Re: [PATCH v3 resend 1/2] mm: Add an F_SEAL_FUTURE_WRITE seal to memfd
From: Joel Fernandes @ 2018-11-09 23:46 UTC (permalink / raw)
To: Jann Horn
Cc: kernel list, jreck, John Stultz, Todd Kjos, Greg Kroah-Hartman,
Christoph Hellwig, Al Viro, Andrew Morton, Daniel Colascione,
Bruce Fields, jlayton, Khalid Aziz, Lei.Yang, linux-fsdevel,
linux-kselftest, Linux-MM, marcandre.lureau, Mike Kravetz,
minchan, shuah, valdis.kletnieks, Hugh Dickins, Linux API
In-Reply-To: <CAG48ez1h=v-JYnDw81HaYJzOfrNhwYksxmc2r=cJvdQVgYM+NA@mail.gmail.com>
On Fri, Nov 09, 2018 at 10:06:31PM +0100, Jann Horn wrote:
> +linux-api for API addition
> +hughd as FYI since this is somewhat related to mm/shmem
>
> On Fri, Nov 9, 2018 at 9:46 PM Joel Fernandes (Google)
> <joel@joelfernandes.org> wrote:
> > Android uses ashmem for sharing memory regions. We are looking forward
> > to migrating all usecases of ashmem to memfd so that we can possibly
> > remove the ashmem driver in the future from staging while also
> > benefiting from using memfd and contributing to it. Note staging drivers
> > are also not ABI and generally can be removed at anytime.
> >
> > One of the main usecases Android has is the ability to create a region
> > and mmap it as writeable, then add protection against making any
> > "future" writes while keeping the existing already mmap'ed
> > writeable-region active. This allows us to implement a usecase where
> > receivers of the shared memory buffer can get a read-only view, while
> > the sender continues to write to the buffer.
> > See CursorWindow documentation in Android for more details:
> > https://developer.android.com/reference/android/database/CursorWindow
> >
> > This usecase cannot be implemented with the existing F_SEAL_WRITE seal.
> > To support the usecase, this patch adds a new F_SEAL_FUTURE_WRITE seal
> > which prevents any future mmap and write syscalls from succeeding while
> > keeping the existing mmap active.
>
> Please CC linux-api@ on patches like this. If you had done that, I
> might have criticized your v1 patch instead of your v3 patch...
Ok, will do from next time.
> > The following program shows the seal
> > working in action:
> [...]
> > Cc: jreck@google.com
> > Cc: john.stultz@linaro.org
> > Cc: tkjos@google.com
> > Cc: gregkh@linuxfoundation.org
> > Cc: hch@infradead.org
> > Reviewed-by: John Stultz <john.stultz@linaro.org>
> > Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
> > ---
> [...]
> > diff --git a/mm/memfd.c b/mm/memfd.c
> > index 2bb5e257080e..5ba9804e9515 100644
> > --- a/mm/memfd.c
> > +++ b/mm/memfd.c
> [...]
> > @@ -219,6 +220,25 @@ static int memfd_add_seals(struct file *file, unsigned int seals)
> > }
> > }
> >
> > + if ((seals & F_SEAL_FUTURE_WRITE) &&
> > + !(*file_seals & F_SEAL_FUTURE_WRITE)) {
> > + /*
> > + * The FUTURE_WRITE seal also prevents growing and shrinking
> > + * so we need them to be already set, or requested now.
> > + */
> > + int test_seals = (seals | *file_seals) &
> > + (F_SEAL_GROW | F_SEAL_SHRINK);
> > +
> > + if (test_seals != (F_SEAL_GROW | F_SEAL_SHRINK)) {
> > + error = -EINVAL;
> > + goto unlock;
> > + }
> > +
> > + spin_lock(&file->f_lock);
> > + file->f_mode &= ~(FMODE_WRITE | FMODE_PWRITE);
> > + spin_unlock(&file->f_lock);
> > + }
>
> So you're fiddling around with the file, but not the inode? How are
> you preventing code like the following from re-opening the file as
> writable?
>
> $ cat memfd.c
> #define _GNU_SOURCE
> #include <unistd.h>
> #include <sys/syscall.h>
> #include <printf.h>
> #include <fcntl.h>
> #include <err.h>
> #include <stdio.h>
>
> int main(void) {
> int fd = syscall(__NR_memfd_create, "testfd", 0);
> if (fd == -1) err(1, "memfd");
> char path[100];
> sprintf(path, "/proc/self/fd/%d", fd);
> int fd2 = open(path, O_RDWR);
> if (fd2 == -1) err(1, "reopen");
> printf("reopen successful: %d\n", fd2);
> }
> $ gcc -o memfd memfd.c
> $ ./memfd
> reopen successful: 4
Great catch and this is indeed an issue :-(. I verified it too.
> That aside: I wonder whether a better API would be something that
> allows you to create a new readonly file descriptor, instead of
> fiddling with the writability of an existing fd.
Android usecases cannot deal with a new fd number because it breaks the
continuity of having the same old fd, as Dan also pointed out.
Also such API will have the same issues you brought up?
thanks,
- Joel
^ permalink raw reply
* Re: [PATCH v3 resend 1/2] mm: Add an F_SEAL_FUTURE_WRITE seal to memfd
From: Joel Fernandes @ 2018-11-10 1:36 UTC (permalink / raw)
To: Andy Lutomirski
Cc: Daniel Colascione, Jann Horn, kernel list, John Reck, John Stultz,
Todd Kjos, Greg Kroah-Hartman, Christoph Hellwig, Al Viro,
Andrew Morton, Bruce Fields, Jeff Layton, Khalid Aziz, Lei.Yang,
linux-fsdevel, linux-kselftest, Linux-MM, marcandre.lureau,
Mike Kravetz, Minchan Kim, Shuah Khan, valdis.kletnieks
In-Reply-To: <F8A6A5DC-3BA0-43BD-B7EC-EDE199B33A02@amacapital.net>
On Fri, Nov 09, 2018 at 03:14:02PM -0800, Andy Lutomirski wrote:
> >>>> That aside: I wonder whether a better API would be something that
> >>>> allows you to create a new readonly file descriptor, instead of
> >>>> fiddling with the writability of an existing fd.
> >>>
> >>> That doesn't work, unfortunately. The ashmem API we're replacing with
> >>> memfd requires file descriptor continuity. I also looked into opening
> >>> a new FD and dup2(2)ing atop the old one, but this approach doesn't
> >>> work in the case that the old FD has already leaked to some other
> >>> context (e.g., another dup, SCM_RIGHTS). See
> >>> https://developer.android.com/ndk/reference/group/memory. We can't
> >>> break ASharedMemory_setProt.
> >>
> >>
> >> Hmm. If we fix the general reopen bug, a way to drop write access from
> >> an existing struct file would do what Android needs, right? I don’t
> >> know if there are general VFS issues with that.
> >
I don't think there is a way to fix this in /proc/pid/fd. At the proc
level, the /proc/pid/fd/N files are just soft symlinks that follow through to
the actual file. The open is actually done on that inode/file. I think
changing it the way being discussed here means changing the way symlinks work
in Linux.
I think the right way to fix this is at the memfd inode level. I am working
on a follow up patch on top of this patch, and will send that out in a few
days (along with the man page updates).
thanks!
- Joel
^ permalink raw reply
* Re: [PATCH v3 resend 1/2] mm: Add an F_SEAL_FUTURE_WRITE seal to memfd
From: Joel Fernandes @ 2018-11-10 1:49 UTC (permalink / raw)
To: Michael Tirado
Cc: Andy Lutomirski, Jann Horn, LKML, jreck, john.stultz, tkjos,
gregkh, hch, viro, Andrew Morton, dancol, bfields, jlayton,
khalid.aziz, Lei.Yang, linux-fsdevel, linux-kselftest, linux-mm,
marcandre.lureau, mike.kravetz, minchan, shuah, valdis.kletnieks,
hughd, linux-api
In-Reply-To: <CAMkWEXOLJ=ymbVjQfA2MD8XA7Y9Lu3ByJYUY-JvpnYKJ5gkY1w@mail.gmail.com>
On Fri, Nov 09, 2018 at 08:02:14PM +0000, Michael Tirado wrote:
[...]
> > > That aside: I wonder whether a better API would be something that
> > > allows you to create a new readonly file descriptor, instead of
> > > fiddling with the writability of an existing fd.
> >
> > Every now and then I try to write a patch to prevent using proc to reopen
> > a file with greater permission than the original open.
> >
> > I like your idea to have a clean way to reopen a a memfd with reduced
> > permissions. But I would make it a syscall instead and maybe make it only
> > work for memfd at first. And the proc issue would need to be fixed, too.
>
> IMO the best solution would handle the issue at memfd creation time by
> removing the race condition.
I agree, this is another idea I'm exploring. We could add a new .open
callback to shmem_file_operations and check for seals there.
thanks,
- Joel
^ permalink raw reply
* Re: [PATCH v3 resend 1/2] mm: Add an F_SEAL_FUTURE_WRITE seal to memfd
From: Joel Fernandes @ 2018-11-10 3:20 UTC (permalink / raw)
To: Jann Horn
Cc: kernel list, jreck, John Stultz, Todd Kjos, Greg Kroah-Hartman,
Christoph Hellwig, Al Viro, Andrew Morton, Daniel Colascione,
Bruce Fields, jlayton, Khalid Aziz, Lei.Yang, linux-fsdevel,
linux-kselftest, Linux-MM, marcandre.lureau, Mike Kravetz,
minchan, shuah, valdis.kletnieks, Hugh Dickins, Linux API
In-Reply-To: <CAG48ez0kQ4d566bXTFOYANDgii-stL-Qj-oyaBzvfxdV=PU-7g@mail.gmail.com>
On Fri, Nov 09, 2018 at 10:19:03PM +0100, Jann Horn wrote:
> On Fri, Nov 9, 2018 at 10:06 PM Jann Horn <jannh@google.com> wrote:
> > On Fri, Nov 9, 2018 at 9:46 PM Joel Fernandes (Google)
> > <joel@joelfernandes.org> wrote:
> > > Android uses ashmem for sharing memory regions. We are looking forward
> > > to migrating all usecases of ashmem to memfd so that we can possibly
> > > remove the ashmem driver in the future from staging while also
> > > benefiting from using memfd and contributing to it. Note staging drivers
> > > are also not ABI and generally can be removed at anytime.
> > >
> > > One of the main usecases Android has is the ability to create a region
> > > and mmap it as writeable, then add protection against making any
> > > "future" writes while keeping the existing already mmap'ed
> > > writeable-region active. This allows us to implement a usecase where
> > > receivers of the shared memory buffer can get a read-only view, while
> > > the sender continues to write to the buffer.
> > > See CursorWindow documentation in Android for more details:
> > > https://developer.android.com/reference/android/database/CursorWindow
> > >
> > > This usecase cannot be implemented with the existing F_SEAL_WRITE seal.
> > > To support the usecase, this patch adds a new F_SEAL_FUTURE_WRITE seal
> > > which prevents any future mmap and write syscalls from succeeding while
> > > keeping the existing mmap active.
> >
> > Please CC linux-api@ on patches like this. If you had done that, I
> > might have criticized your v1 patch instead of your v3 patch...
> >
> > > The following program shows the seal
> > > working in action:
> > [...]
> > > Cc: jreck@google.com
> > > Cc: john.stultz@linaro.org
> > > Cc: tkjos@google.com
> > > Cc: gregkh@linuxfoundation.org
> > > Cc: hch@infradead.org
> > > Reviewed-by: John Stultz <john.stultz@linaro.org>
> > > Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
> > > ---
> > [...]
> > > diff --git a/mm/memfd.c b/mm/memfd.c
> > > index 2bb5e257080e..5ba9804e9515 100644
> > > --- a/mm/memfd.c
> > > +++ b/mm/memfd.c
> > [...]
> > > @@ -219,6 +220,25 @@ static int memfd_add_seals(struct file *file, unsigned int seals)
> > > }
> > > }
> > >
> > > + if ((seals & F_SEAL_FUTURE_WRITE) &&
> > > + !(*file_seals & F_SEAL_FUTURE_WRITE)) {
> > > + /*
> > > + * The FUTURE_WRITE seal also prevents growing and shrinking
> > > + * so we need them to be already set, or requested now.
> > > + */
> > > + int test_seals = (seals | *file_seals) &
> > > + (F_SEAL_GROW | F_SEAL_SHRINK);
> > > +
> > > + if (test_seals != (F_SEAL_GROW | F_SEAL_SHRINK)) {
> > > + error = -EINVAL;
> > > + goto unlock;
> > > + }
> > > +
> > > + spin_lock(&file->f_lock);
> > > + file->f_mode &= ~(FMODE_WRITE | FMODE_PWRITE);
> > > + spin_unlock(&file->f_lock);
> > > + }
> >
> > So you're fiddling around with the file, but not the inode? How are
> > you preventing code like the following from re-opening the file as
> > writable?
> >
> > $ cat memfd.c
> > #define _GNU_SOURCE
> > #include <unistd.h>
> > #include <sys/syscall.h>
> > #include <printf.h>
> > #include <fcntl.h>
> > #include <err.h>
> > #include <stdio.h>
> >
> > int main(void) {
> > int fd = syscall(__NR_memfd_create, "testfd", 0);
> > if (fd == -1) err(1, "memfd");
> > char path[100];
> > sprintf(path, "/proc/self/fd/%d", fd);
> > int fd2 = open(path, O_RDWR);
> > if (fd2 == -1) err(1, "reopen");
> > printf("reopen successful: %d\n", fd2);
> > }
> > $ gcc -o memfd memfd.c
> > $ ./memfd
> > reopen successful: 4
> > $
> >
> > That aside: I wonder whether a better API would be something that
> > allows you to create a new readonly file descriptor, instead of
> > fiddling with the writability of an existing fd.
>
> My favorite approach would be to forbid open() on memfds, hope that
> nobody notices the tiny API break, and then add an ioctl for "reopen
> this memfd with reduced permissions" - but that's just my personal
> opinion.
I did something along these lines and it fixes the issue, but I forbid open
of memfd only when the F_SEAL_FUTURE_WRITE seal is in place. So then its not
an ABI break because this is a brand new seal. That seems the least intrusive
solution and it works. Do you mind testing it and I'll add your and Tested-by
to the new fix? The patch is based on top of this series.
---8<-----------
From: "Joel Fernandes (Google)" <joel@joelfernandes.org>
Subject: [PATCH] mm/memfd: Fix possible promotion to writeable of sealed memfd
Jann Horn found that reopening an F_SEAL_FUTURE_WRITE sealed memfd
through /proc/self/fd/N symlink as writeable succeeds. The simplest fix
without causing ABI breakages and ugly VFS hacks is to simply deny all
opens on F_SEAL_FUTURE_WRITE sealed fds.
Reported-by: Jann Horn <jannh@google.com>
Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
---
mm/shmem.c | 18 ++++++++++++++++++
1 file changed, 18 insertions(+)
diff --git a/mm/shmem.c b/mm/shmem.c
index 446942677cd4..5b378c486b8f 100644
--- a/mm/shmem.c
+++ b/mm/shmem.c
@@ -3611,7 +3611,25 @@ static const struct address_space_operations shmem_aops = {
.error_remove_page = generic_error_remove_page,
};
+/* Could arrive here for memfds opened through /proc/ */
+int shmem_open(struct inode *inode, struct file *file)
+{
+ struct shmem_inode_info *info = SHMEM_I(inode);
+
+ /*
+ * memfds for which future writes have been prevented
+ * should not be reopened, say, through /proc/pid/fd/N
+ * symlinks otherwise it can cause a sealed memfd to be
+ * promoted to writable.
+ */
+ if (info->seals & F_SEAL_FUTURE_WRITE)
+ return -EACCES;
+
+ return 0;
+}
+
static const struct file_operations shmem_file_operations = {
+ .open = shmem_open,
.mmap = shmem_mmap,
.get_unmapped_area = shmem_get_unmapped_area,
#ifdef CONFIG_TMPFS
--
2.19.1.930.g4563a0d9d0-goog
^ permalink raw reply related
* Re: [PATCH v3 resend 1/2] mm: Add an F_SEAL_FUTURE_WRITE seal to memfd
From: Andy Lutomirski @ 2018-11-10 6:05 UTC (permalink / raw)
To: Joel Fernandes
Cc: Jann Horn, kernel list, jreck, John Stultz, Todd Kjos,
Greg Kroah-Hartman, Christoph Hellwig, Al Viro, Andrew Morton,
Daniel Colascione, Bruce Fields, jlayton, Khalid Aziz, Lei.Yang,
linux-fsdevel, linux-kselftest, Linux-MM, marcandre.lureau,
Mike Kravetz, minchan, shuah, valdis.kletnieks, Hugh Dickins,
Linux API
In-Reply-To: <20181110032005.GA22238@google.com>
> On Nov 9, 2018, at 7:20 PM, Joel Fernandes <joel@joelfernandes.org> wrote:
>
>> On Fri, Nov 09, 2018 at 10:19:03PM +0100, Jann Horn wrote:
>>> On Fri, Nov 9, 2018 at 10:06 PM Jann Horn <jannh@google.com> wrote:
>>> On Fri, Nov 9, 2018 at 9:46 PM Joel Fernandes (Google)
>>> <joel@joelfernandes.org> wrote:
>>>> Android uses ashmem for sharing memory regions. We are looking forward
>>>> to migrating all usecases of ashmem to memfd so that we can possibly
>>>> remove the ashmem driver in the future from staging while also
>>>> benefiting from using memfd and contributing to it. Note staging drivers
>>>> are also not ABI and generally can be removed at anytime.
>>>>
>>>> One of the main usecases Android has is the ability to create a region
>>>> and mmap it as writeable, then add protection against making any
>>>> "future" writes while keeping the existing already mmap'ed
>>>> writeable-region active. This allows us to implement a usecase where
>>>> receivers of the shared memory buffer can get a read-only view, while
>>>> the sender continues to write to the buffer.
>>>> See CursorWindow documentation in Android for more details:
>>>> https://developer.android.com/reference/android/database/CursorWindow
>>>>
>>>> This usecase cannot be implemented with the existing F_SEAL_WRITE seal.
>>>> To support the usecase, this patch adds a new F_SEAL_FUTURE_WRITE seal
>>>> which prevents any future mmap and write syscalls from succeeding while
>>>> keeping the existing mmap active.
>>>
>>> Please CC linux-api@ on patches like this. If you had done that, I
>>> might have criticized your v1 patch instead of your v3 patch...
>>>
>>>> The following program shows the seal
>>>> working in action:
>>> [...]
>>>> Cc: jreck@google.com
>>>> Cc: john.stultz@linaro.org
>>>> Cc: tkjos@google.com
>>>> Cc: gregkh@linuxfoundation.org
>>>> Cc: hch@infradead.org
>>>> Reviewed-by: John Stultz <john.stultz@linaro.org>
>>>> Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
>>>> ---
>>> [...]
>>>> diff --git a/mm/memfd.c b/mm/memfd.c
>>>> index 2bb5e257080e..5ba9804e9515 100644
>>>> --- a/mm/memfd.c
>>>> +++ b/mm/memfd.c
>>> [...]
>>>> @@ -219,6 +220,25 @@ static int memfd_add_seals(struct file *file, unsigned int seals)
>>>> }
>>>> }
>>>>
>>>> + if ((seals & F_SEAL_FUTURE_WRITE) &&
>>>> + !(*file_seals & F_SEAL_FUTURE_WRITE)) {
>>>> + /*
>>>> + * The FUTURE_WRITE seal also prevents growing and shrinking
>>>> + * so we need them to be already set, or requested now.
>>>> + */
>>>> + int test_seals = (seals | *file_seals) &
>>>> + (F_SEAL_GROW | F_SEAL_SHRINK);
>>>> +
>>>> + if (test_seals != (F_SEAL_GROW | F_SEAL_SHRINK)) {
>>>> + error = -EINVAL;
>>>> + goto unlock;
>>>> + }
>>>> +
>>>> + spin_lock(&file->f_lock);
>>>> + file->f_mode &= ~(FMODE_WRITE | FMODE_PWRITE);
>>>> + spin_unlock(&file->f_lock);
>>>> + }
>>>
>>> So you're fiddling around with the file, but not the inode? How are
>>> you preventing code like the following from re-opening the file as
>>> writable?
>>>
>>> $ cat memfd.c
>>> #define _GNU_SOURCE
>>> #include <unistd.h>
>>> #include <sys/syscall.h>
>>> #include <printf.h>
>>> #include <fcntl.h>
>>> #include <err.h>
>>> #include <stdio.h>
>>>
>>> int main(void) {
>>> int fd = syscall(__NR_memfd_create, "testfd", 0);
>>> if (fd == -1) err(1, "memfd");
>>> char path[100];
>>> sprintf(path, "/proc/self/fd/%d", fd);
>>> int fd2 = open(path, O_RDWR);
>>> if (fd2 == -1) err(1, "reopen");
>>> printf("reopen successful: %d\n", fd2);
>>> }
>>> $ gcc -o memfd memfd.c
>>> $ ./memfd
>>> reopen successful: 4
>>> $
>>>
>>> That aside: I wonder whether a better API would be something that
>>> allows you to create a new readonly file descriptor, instead of
>>> fiddling with the writability of an existing fd.
>>
>> My favorite approach would be to forbid open() on memfds, hope that
>> nobody notices the tiny API break, and then add an ioctl for "reopen
>> this memfd with reduced permissions" - but that's just my personal
>> opinion.
>
> I did something along these lines and it fixes the issue, but I forbid open
> of memfd only when the F_SEAL_FUTURE_WRITE seal is in place. So then its not
> an ABI break because this is a brand new seal. That seems the least intrusive
> solution and it works. Do you mind testing it and I'll add your and Tested-by
> to the new fix? The patch is based on top of this series.
>
> ---8<-----------
> From: "Joel Fernandes (Google)" <joel@joelfernandes.org>
> Subject: [PATCH] mm/memfd: Fix possible promotion to writeable of sealed memfd
>
> Jann Horn found that reopening an F_SEAL_FUTURE_WRITE sealed memfd
> through /proc/self/fd/N symlink as writeable succeeds. The simplest fix
> without causing ABI breakages and ugly VFS hacks is to simply deny all
> opens on F_SEAL_FUTURE_WRITE sealed fds.
>
> Reported-by: Jann Horn <jannh@google.com>
> Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
> ---
> mm/shmem.c | 18 ++++++++++++++++++
> 1 file changed, 18 insertions(+)
>
> diff --git a/mm/shmem.c b/mm/shmem.c
> index 446942677cd4..5b378c486b8f 100644
> --- a/mm/shmem.c
> +++ b/mm/shmem.c
> @@ -3611,7 +3611,25 @@ static const struct address_space_operations shmem_aops = {
> .error_remove_page = generic_error_remove_page,
> };
>
> +/* Could arrive here for memfds opened through /proc/ */
> +int shmem_open(struct inode *inode, struct file *file)
> +{
> + struct shmem_inode_info *info = SHMEM_I(inode);
> +
> + /*
> + * memfds for which future writes have been prevented
> + * should not be reopened, say, through /proc/pid/fd/N
> + * symlinks otherwise it can cause a sealed memfd to be
> + * promoted to writable.
> + */
> + if (info->seals & F_SEAL_FUTURE_WRITE)
> + return -EACCES;
> +
> + return 0;
> +}
The result of this series is very warty. We have a concept of seals, and they all work similarly, except the new future write seal. That one:
- causes a probably-observable effect in the file mode in F_GETFL.
- causes reopen to fail.
- does *not* affect other struct files that may already exist on the same inode.
- mysteriously malfunctions if you try to set it again on another struct file that already exists
- probably is insecure when used on hugetlbfs.
I see two reasonable solutions:
1. Don’t fiddle with the struct file at all. Instead make the inode flag work by itself.
2. Don’t call it a “seal”. Instead fix the /proc hole and add an API to drop write access on an existing struct file.
I personally prefer #2.
> +
> static const struct file_operations shmem_file_operations = {
> + .open = shmem_open,
> .mmap = shmem_mmap,
> .get_unmapped_area = shmem_get_unmapped_area,
> #ifdef CONFIG_TMPFS
> --
> 2.19.1.930.g4563a0d9d0-goog
>
^ permalink raw reply
* Re: [PATCH v3 resend 1/2] mm: Add an F_SEAL_FUTURE_WRITE seal to memfd
From: Joel Fernandes @ 2018-11-10 17:10 UTC (permalink / raw)
To: Daniel Colascione
Cc: Jann Horn, kernel list, jreck@google.com, John Stultz, Todd Kjos,
Greg Kroah-Hartman, Christoph Hellwig, Al Viro, Andrew Morton,
Bruce Fields, jlayton@kernel.org, Khalid Aziz,
Lei.Yang@windriver.com, linux-fsdevel@vger.kernel.org,
linux-kselftest@vger.kernel.org, Linux-MM,
marcandre.lureau@redhat.com, Mike Kravetz <mike>
In-Reply-To: <CAKOZuethC7+YrRyyGciUCfhSSa9cCcAFJ8g_qEw9uh3TBbyOcg@mail.gmail.com>
On Sat, Nov 10, 2018 at 04:26:46AM -0800, Daniel Colascione wrote:
> On Friday, November 9, 2018, Joel Fernandes <joel@joelfernandes.org> wrote:
>
> > On Fri, Nov 09, 2018 at 10:19:03PM +0100, Jann Horn wrote:
> > > On Fri, Nov 9, 2018 at 10:06 PM Jann Horn <jannh@google.com> wrote:
> > > > On Fri, Nov 9, 2018 at 9:46 PM Joel Fernandes (Google)
> > > > <joel@joelfernandes.org> wrote:
> > > > > Android uses ashmem for sharing memory regions. We are looking
> > forward
> > > > > to migrating all usecases of ashmem to memfd so that we can possibly
> > > > > remove the ashmem driver in the future from staging while also
> > > > > benefiting from using memfd and contributing to it. Note staging
> > drivers
> > > > > are also not ABI and generally can be removed at anytime.
> > > > >
> > > > > One of the main usecases Android has is the ability to create a
> > region
> > > > > and mmap it as writeable, then add protection against making any
> > > > > "future" writes while keeping the existing already mmap'ed
> > > > > writeable-region active. This allows us to implement a usecase where
> > > > > receivers of the shared memory buffer can get a read-only view, while
> > > > > the sender continues to write to the buffer.
> > > > > See CursorWindow documentation in Android for more details:
> > > > > https://developer.android.com/reference/android/database/
> > CursorWindow
> > > > >
> > > > > This usecase cannot be implemented with the existing F_SEAL_WRITE
> > seal.
> > > > > To support the usecase, this patch adds a new F_SEAL_FUTURE_WRITE
> > seal
> > > > > which prevents any future mmap and write syscalls from succeeding
> > while
> > > > > keeping the existing mmap active.
> > > >
> > > > Please CC linux-api@ on patches like this. If you had done that, I
> > > > might have criticized your v1 patch instead of your v3 patch...
> > > >
> > > > > The following program shows the seal
> > > > > working in action:
> > > > [...]
> > > > > Cc: jreck@google.com
> > > > > Cc: john.stultz@linaro.org
> > > > > Cc: tkjos@google.com
> > > > > Cc: gregkh@linuxfoundation.org
> > > > > Cc: hch@infradead.org
> > > > > Reviewed-by: John Stultz <john.stultz@linaro.org>
> > > > > Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
> > > > > ---
> > > > [...]
> > > > > diff --git a/mm/memfd.c b/mm/memfd.c
> > > > > index 2bb5e257080e..5ba9804e9515 100644
> > > > > --- a/mm/memfd.c
> > > > > +++ b/mm/memfd.c
> > > > [...]
> > > > > @@ -219,6 +220,25 @@ static int memfd_add_seals(struct file *file,
> > unsigned int seals)
> > > > > }
> > > > > }
> > > > >
> > > > > + if ((seals & F_SEAL_FUTURE_WRITE) &&
> > > > > + !(*file_seals & F_SEAL_FUTURE_WRITE)) {
> > > > > + /*
> > > > > + * The FUTURE_WRITE seal also prevents growing and
> > shrinking
> > > > > + * so we need them to be already set, or requested
> > now.
> > > > > + */
> > > > > + int test_seals = (seals | *file_seals) &
> > > > > + (F_SEAL_GROW | F_SEAL_SHRINK);
> > > > > +
> > > > > + if (test_seals != (F_SEAL_GROW | F_SEAL_SHRINK)) {
> > > > > + error = -EINVAL;
> > > > > + goto unlock;
> > > > > + }
> > > > > +
> > > > > + spin_lock(&file->f_lock);
> > > > > + file->f_mode &= ~(FMODE_WRITE | FMODE_PWRITE);
> > > > > + spin_unlock(&file->f_lock);
> > > > > + }
> > > >
> > > > So you're fiddling around with the file, but not the inode? How are
> > > > you preventing code like the following from re-opening the file as
> > > > writable?
> > > >
> > > > $ cat memfd.c
> > > > #define _GNU_SOURCE
> > > > #include <unistd.h>
> > > > #include <sys/syscall.h>
> > > > #include <printf.h>
> > > > #include <fcntl.h>
> > > > #include <err.h>
> > > > #include <stdio.h>
> > > >
> > > > int main(void) {
> > > > int fd = syscall(__NR_memfd_create, "testfd", 0);
> > > > if (fd == -1) err(1, "memfd");
> > > > char path[100];
> > > > sprintf(path, "/proc/self/fd/%d", fd);
> > > > int fd2 = open(path, O_RDWR);
> > > > if (fd2 == -1) err(1, "reopen");
> > > > printf("reopen successful: %d\n", fd2);
> > > > }
> > > > $ gcc -o memfd memfd.c
> > > > $ ./memfd
> > > > reopen successful: 4
> > > > $
> > > >
> > > > That aside: I wonder whether a better API would be something that
> > > > allows you to create a new readonly file descriptor, instead of
> > > > fiddling with the writability of an existing fd.
> > >
> > > My favorite approach would be to forbid open() on memfds, hope that
> > > nobody notices the tiny API break, and then add an ioctl for "reopen
> > > this memfd with reduced permissions" - but that's just my personal
> > > opinion.
> >
> > I did something along these lines and it fixes the issue, but I forbid open
> > of memfd only when the F_SEAL_FUTURE_WRITE seal is in place. So then its
> > not
> > an ABI break because this is a brand new seal. That seems the least
> > intrusive
> > solution and it works. Do you mind testing it and I'll add your and
> > Tested-by
> > to the new fix? The patch is based on top of this series.
> >
>
> Please don't forbid reopens entirely. You're taking a feature that works
> generally (reopens) and breaking it in one specific case (memfd write
> sealed files). The open modes are available in .open in the struct file:
> you can deny *only* opens for write instead of denying reopens generally.
Yes, as we discussed over chat already, I will implement it that way.
Also lets continue to discuss Andy's concerns he raised on the other thread.
thanks,
- Joel
^ permalink raw reply
* Re: [PATCH v3 resend 1/2] mm: Add an F_SEAL_FUTURE_WRITE seal to memfd
From: Joel Fernandes @ 2018-11-10 18:24 UTC (permalink / raw)
To: Andy Lutomirski
Cc: Jann Horn, kernel list, jreck, John Stultz, Todd Kjos,
Greg Kroah-Hartman, Christoph Hellwig, Al Viro, Andrew Morton,
Daniel Colascione, Bruce Fields, jlayton, Khalid Aziz, Lei.Yang,
linux-fsdevel, linux-kselftest, Linux-MM, marcandre.lureau,
Mike Kravetz, minchan, shuah, valdis.kletnieks, Hugh Dickins,
Linux API
In-Reply-To: <69CE06CC-E47C-4992-848A-66EB23EE6C74@amacapital.net>
Thanks Andy for your thoughts, my comments below:
On Fri, Nov 09, 2018 at 10:05:14PM -0800, Andy Lutomirski wrote:
>
>
> > On Nov 9, 2018, at 7:20 PM, Joel Fernandes <joel@joelfernandes.org> wrote:
> >
> >> On Fri, Nov 09, 2018 at 10:19:03PM +0100, Jann Horn wrote:
> >>> On Fri, Nov 9, 2018 at 10:06 PM Jann Horn <jannh@google.com> wrote:
> >>> On Fri, Nov 9, 2018 at 9:46 PM Joel Fernandes (Google)
> >>> <joel@joelfernandes.org> wrote:
> >>>> Android uses ashmem for sharing memory regions. We are looking forward
> >>>> to migrating all usecases of ashmem to memfd so that we can possibly
> >>>> remove the ashmem driver in the future from staging while also
> >>>> benefiting from using memfd and contributing to it. Note staging drivers
> >>>> are also not ABI and generally can be removed at anytime.
> >>>>
> >>>> One of the main usecases Android has is the ability to create a region
> >>>> and mmap it as writeable, then add protection against making any
> >>>> "future" writes while keeping the existing already mmap'ed
> >>>> writeable-region active. This allows us to implement a usecase where
> >>>> receivers of the shared memory buffer can get a read-only view, while
> >>>> the sender continues to write to the buffer.
> >>>> See CursorWindow documentation in Android for more details:
> >>>> https://developer.android.com/reference/android/database/CursorWindow
> >>>>
> >>>> This usecase cannot be implemented with the existing F_SEAL_WRITE seal.
> >>>> To support the usecase, this patch adds a new F_SEAL_FUTURE_WRITE seal
> >>>> which prevents any future mmap and write syscalls from succeeding while
> >>>> keeping the existing mmap active.
> >>>
> >>> Please CC linux-api@ on patches like this. If you had done that, I
> >>> might have criticized your v1 patch instead of your v3 patch...
> >>>
> >>>> The following program shows the seal
> >>>> working in action:
> >>> [...]
> >>>> Cc: jreck@google.com
> >>>> Cc: john.stultz@linaro.org
> >>>> Cc: tkjos@google.com
> >>>> Cc: gregkh@linuxfoundation.org
> >>>> Cc: hch@infradead.org
> >>>> Reviewed-by: John Stultz <john.stultz@linaro.org>
> >>>> Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
> >>>> ---
> >>> [...]
> >>>> diff --git a/mm/memfd.c b/mm/memfd.c
> >>>> index 2bb5e257080e..5ba9804e9515 100644
> >>>> --- a/mm/memfd.c
> >>>> +++ b/mm/memfd.c
> >>> [...]
> >>>> @@ -219,6 +220,25 @@ static int memfd_add_seals(struct file *file, unsigned int seals)
> >>>> }
> >>>> }
> >>>>
> >>>> + if ((seals & F_SEAL_FUTURE_WRITE) &&
> >>>> + !(*file_seals & F_SEAL_FUTURE_WRITE)) {
> >>>> + /*
> >>>> + * The FUTURE_WRITE seal also prevents growing and shrinking
> >>>> + * so we need them to be already set, or requested now.
> >>>> + */
> >>>> + int test_seals = (seals | *file_seals) &
> >>>> + (F_SEAL_GROW | F_SEAL_SHRINK);
> >>>> +
> >>>> + if (test_seals != (F_SEAL_GROW | F_SEAL_SHRINK)) {
> >>>> + error = -EINVAL;
> >>>> + goto unlock;
> >>>> + }
> >>>> +
> >>>> + spin_lock(&file->f_lock);
> >>>> + file->f_mode &= ~(FMODE_WRITE | FMODE_PWRITE);
> >>>> + spin_unlock(&file->f_lock);
> >>>> + }
> >>>
> >>> So you're fiddling around with the file, but not the inode? How are
> >>> you preventing code like the following from re-opening the file as
> >>> writable?
> >>>
> >>> $ cat memfd.c
> >>> #define _GNU_SOURCE
> >>> #include <unistd.h>
> >>> #include <sys/syscall.h>
> >>> #include <printf.h>
> >>> #include <fcntl.h>
> >>> #include <err.h>
> >>> #include <stdio.h>
> >>>
> >>> int main(void) {
> >>> int fd = syscall(__NR_memfd_create, "testfd", 0);
> >>> if (fd == -1) err(1, "memfd");
> >>> char path[100];
> >>> sprintf(path, "/proc/self/fd/%d", fd);
> >>> int fd2 = open(path, O_RDWR);
> >>> if (fd2 == -1) err(1, "reopen");
> >>> printf("reopen successful: %d\n", fd2);
> >>> }
> >>> $ gcc -o memfd memfd.c
> >>> $ ./memfd
> >>> reopen successful: 4
> >>> $
> >>>
> >>> That aside: I wonder whether a better API would be something that
> >>> allows you to create a new readonly file descriptor, instead of
> >>> fiddling with the writability of an existing fd.
> >>
> >> My favorite approach would be to forbid open() on memfds, hope that
> >> nobody notices the tiny API break, and then add an ioctl for "reopen
> >> this memfd with reduced permissions" - but that's just my personal
> >> opinion.
> >
> > I did something along these lines and it fixes the issue, but I forbid open
> > of memfd only when the F_SEAL_FUTURE_WRITE seal is in place. So then its not
> > an ABI break because this is a brand new seal. That seems the least intrusive
> > solution and it works. Do you mind testing it and I'll add your and Tested-by
> > to the new fix? The patch is based on top of this series.
> >
> > ---8<-----------
> > From: "Joel Fernandes (Google)" <joel@joelfernandes.org>
> > Subject: [PATCH] mm/memfd: Fix possible promotion to writeable of sealed memfd
> >
> > Jann Horn found that reopening an F_SEAL_FUTURE_WRITE sealed memfd
> > through /proc/self/fd/N symlink as writeable succeeds. The simplest fix
> > without causing ABI breakages and ugly VFS hacks is to simply deny all
> > opens on F_SEAL_FUTURE_WRITE sealed fds.
> >
> > Reported-by: Jann Horn <jannh@google.com>
> > Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
> > ---
> > mm/shmem.c | 18 ++++++++++++++++++
> > 1 file changed, 18 insertions(+)
> >
> > diff --git a/mm/shmem.c b/mm/shmem.c
> > index 446942677cd4..5b378c486b8f 100644
> > --- a/mm/shmem.c
> > +++ b/mm/shmem.c
> > @@ -3611,7 +3611,25 @@ static const struct address_space_operations shmem_aops = {
> > .error_remove_page = generic_error_remove_page,
> > };
> >
> > +/* Could arrive here for memfds opened through /proc/ */
> > +int shmem_open(struct inode *inode, struct file *file)
> > +{
> > + struct shmem_inode_info *info = SHMEM_I(inode);
> > +
> > + /*
> > + * memfds for which future writes have been prevented
> > + * should not be reopened, say, through /proc/pid/fd/N
> > + * symlinks otherwise it can cause a sealed memfd to be
> > + * promoted to writable.
> > + */
> > + if (info->seals & F_SEAL_FUTURE_WRITE)
> > + return -EACCES;
> > +
> > + return 0;
> > +}
>
> The result of this series is very warty. We have a concept of seals, and they all work similarly, except the new future write seal. That one:
>
I don't see it as warty, different seals will work differently. It works
quite well for our usecase, and since Linux is all about solving real
problems in the real work, it would be useful to have it.
> - causes a probably-observable effect in the file mode in F_GETFL.
Wouldn't that be the right thing to observe anyway?
> - causes reopen to fail.
So this concern isn't true anymore if we make reopen fail only for WRITE
opens as Daniel suggested. I will make this change so that the security fix
is a clean one.
> - does *not* affect other struct files that may already exist on the same inode.
TBH if you really want to block all writes to the file, then you want
F_SEAL_WRITE, not this seal. The usecase we have is the fd is sent over IPC
to another process and we want to prevent any new writes in the receiver
side. There is no way this other receiving process can have an existing fd
unless it was already sent one without the seal applied. The proposed seal
could be renamed to F_SEAL_FD_WRITE if that is preferred.
> - mysteriously malfunctions if you try to set it again on another struct
> file that already exists
>
I didn't follow this, could you explain more?
> - probably is insecure when used on hugetlbfs.
The usecase is not expected to prevent all writes, indeed the usecase
requires existing mmaps to continue to be able to write into the memory map.
So would you call that a security issue too? The use of the seal wants to
allow existing mmap regions to be continue to be written into (I mentioned
more details in the cover letter).
> I see two reasonable solutions:
>
> 1. Don’t fiddle with the struct file at all. Instead make the inode flag
> work by itself.
Currently, the various VFS paths check only the struct file's f_mode to deny
writes of already opened files. This would mean more checking in all those
paths (and modification of all those paths).
Anyway going with that idea, we could
1. call deny_write_access(file) from the memfd's seal path which decrements
the inode::i_writecount.
2. call get_write_access(inode) in the various VFS paths in addition to
checking for FMODE_*WRITE and deny the write (incase i_writecount is negative)
That will prevent both reopens, and writes from succeeding. However I worry a
bit about 2 not being too familiar with VFS internals, about what the
consequences of doing that may be.
> 2. Don’t call it a “seal”. Instead fix the /proc hole and add an API to
> drop write access on an existing struct file.
>
> I personally prefer #2.
So I don't think proc has a hole looking at the code yesterday. As I was
saying in other thread, its just how symlinks work. If we were to apply this
API to files in general, and we had symlinks to files and we tried to reopen the
file, we would run into the same issue - that's why I believe it has nothing
to do with proc, and the fix has to be in the underlying inode being pointed
to. Does that make sense or did I miss something?
thanks,
- Joel
^ permalink raw reply
* Re: [PATCH v3 resend 1/2] mm: Add an F_SEAL_FUTURE_WRITE seal to memfd
From: Daniel Colascione @ 2018-11-10 18:45 UTC (permalink / raw)
To: Joel Fernandes
Cc: Andy Lutomirski, Jann Horn, kernel list, John Reck, John Stultz,
Todd Kjos, Greg Kroah-Hartman, Christoph Hellwig, Al Viro,
Andrew Morton, Bruce Fields, Jeff Layton, Khalid Aziz, Lei.Yang,
linux-fsdevel, linux-kselftest, Linux-MM, marcandre.lureau,
Mike Kravetz, Minchan Kim, Shuah Khan, Valdis
In-Reply-To: <20181110182405.GB242356@google.com>
On Sat, Nov 10, 2018 at 10:24 AM, Joel Fernandes <joel@joelfernandes.org> wrote:
> Thanks Andy for your thoughts, my comments below:
>
> On Fri, Nov 09, 2018 at 10:05:14PM -0800, Andy Lutomirski wrote:
>>
>>
>> > On Nov 9, 2018, at 7:20 PM, Joel Fernandes <joel@joelfernandes.org> wrote:
>> >
>> >> On Fri, Nov 09, 2018 at 10:19:03PM +0100, Jann Horn wrote:
>> >>> On Fri, Nov 9, 2018 at 10:06 PM Jann Horn <jannh@google.com> wrote:
>> >>> On Fri, Nov 9, 2018 at 9:46 PM Joel Fernandes (Google)
>> >>> <joel@joelfernandes.org> wrote:
>> >>>> Android uses ashmem for sharing memory regions. We are looking forward
>> >>>> to migrating all usecases of ashmem to memfd so that we can possibly
>> >>>> remove the ashmem driver in the future from staging while also
>> >>>> benefiting from using memfd and contributing to it. Note staging drivers
>> >>>> are also not ABI and generally can be removed at anytime.
>> >>>>
>> >>>> One of the main usecases Android has is the ability to create a region
>> >>>> and mmap it as writeable, then add protection against making any
>> >>>> "future" writes while keeping the existing already mmap'ed
>> >>>> writeable-region active. This allows us to implement a usecase where
>> >>>> receivers of the shared memory buffer can get a read-only view, while
>> >>>> the sender continues to write to the buffer.
>> >>>> See CursorWindow documentation in Android for more details:
>> >>>> https://developer.android.com/reference/android/database/CursorWindow
>> >>>>
>> >>>> This usecase cannot be implemented with the existing F_SEAL_WRITE seal.
>> >>>> To support the usecase, this patch adds a new F_SEAL_FUTURE_WRITE seal
>> >>>> which prevents any future mmap and write syscalls from succeeding while
>> >>>> keeping the existing mmap active.
>> >>>
>> >>> Please CC linux-api@ on patches like this. If you had done that, I
>> >>> might have criticized your v1 patch instead of your v3 patch...
>> >>>
>> >>>> The following program shows the seal
>> >>>> working in action:
>> >>> [...]
>> >>>> Cc: jreck@google.com
>> >>>> Cc: john.stultz@linaro.org
>> >>>> Cc: tkjos@google.com
>> >>>> Cc: gregkh@linuxfoundation.org
>> >>>> Cc: hch@infradead.org
>> >>>> Reviewed-by: John Stultz <john.stultz@linaro.org>
>> >>>> Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
>> >>>> ---
>> >>> [...]
>> >>>> diff --git a/mm/memfd.c b/mm/memfd.c
>> >>>> index 2bb5e257080e..5ba9804e9515 100644
>> >>>> --- a/mm/memfd.c
>> >>>> +++ b/mm/memfd.c
>> >>> [...]
>> >>>> @@ -219,6 +220,25 @@ static int memfd_add_seals(struct file *file, unsigned int seals)
>> >>>> }
>> >>>> }
>> >>>>
>> >>>> + if ((seals & F_SEAL_FUTURE_WRITE) &&
>> >>>> + !(*file_seals & F_SEAL_FUTURE_WRITE)) {
>> >>>> + /*
>> >>>> + * The FUTURE_WRITE seal also prevents growing and shrinking
>> >>>> + * so we need them to be already set, or requested now.
>> >>>> + */
>> >>>> + int test_seals = (seals | *file_seals) &
>> >>>> + (F_SEAL_GROW | F_SEAL_SHRINK);
>> >>>> +
>> >>>> + if (test_seals != (F_SEAL_GROW | F_SEAL_SHRINK)) {
>> >>>> + error = -EINVAL;
>> >>>> + goto unlock;
>> >>>> + }
>> >>>> +
>> >>>> + spin_lock(&file->f_lock);
>> >>>> + file->f_mode &= ~(FMODE_WRITE | FMODE_PWRITE);
>> >>>> + spin_unlock(&file->f_lock);
>> >>>> + }
>> >>>
>> >>> So you're fiddling around with the file, but not the inode? How are
>> >>> you preventing code like the following from re-opening the file as
>> >>> writable?
>> >>>
>> >>> $ cat memfd.c
>> >>> #define _GNU_SOURCE
>> >>> #include <unistd.h>
>> >>> #include <sys/syscall.h>
>> >>> #include <printf.h>
>> >>> #include <fcntl.h>
>> >>> #include <err.h>
>> >>> #include <stdio.h>
>> >>>
>> >>> int main(void) {
>> >>> int fd = syscall(__NR_memfd_create, "testfd", 0);
>> >>> if (fd == -1) err(1, "memfd");
>> >>> char path[100];
>> >>> sprintf(path, "/proc/self/fd/%d", fd);
>> >>> int fd2 = open(path, O_RDWR);
>> >>> if (fd2 == -1) err(1, "reopen");
>> >>> printf("reopen successful: %d\n", fd2);
>> >>> }
>> >>> $ gcc -o memfd memfd.c
>> >>> $ ./memfd
>> >>> reopen successful: 4
>> >>> $
>> >>>
>> >>> That aside: I wonder whether a better API would be something that
>> >>> allows you to create a new readonly file descriptor, instead of
>> >>> fiddling with the writability of an existing fd.
>> >>
>> >> My favorite approach would be to forbid open() on memfds, hope that
>> >> nobody notices the tiny API break, and then add an ioctl for "reopen
>> >> this memfd with reduced permissions" - but that's just my personal
>> >> opinion.
>> >
>> > I did something along these lines and it fixes the issue, but I forbid open
>> > of memfd only when the F_SEAL_FUTURE_WRITE seal is in place. So then its not
>> > an ABI break because this is a brand new seal. That seems the least intrusive
>> > solution and it works. Do you mind testing it and I'll add your and Tested-by
>> > to the new fix? The patch is based on top of this series.
>> >
>> > ---8<-----------
>> > From: "Joel Fernandes (Google)" <joel@joelfernandes.org>
>> > Subject: [PATCH] mm/memfd: Fix possible promotion to writeable of sealed memfd
>> >
>> > Jann Horn found that reopening an F_SEAL_FUTURE_WRITE sealed memfd
>> > through /proc/self/fd/N symlink as writeable succeeds. The simplest fix
>> > without causing ABI breakages and ugly VFS hacks is to simply deny all
>> > opens on F_SEAL_FUTURE_WRITE sealed fds.
>> >
>> > Reported-by: Jann Horn <jannh@google.com>
>> > Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
>> > ---
>> > mm/shmem.c | 18 ++++++++++++++++++
>> > 1 file changed, 18 insertions(+)
>> >
>> > diff --git a/mm/shmem.c b/mm/shmem.c
>> > index 446942677cd4..5b378c486b8f 100644
>> > --- a/mm/shmem.c
>> > +++ b/mm/shmem.c
>> > @@ -3611,7 +3611,25 @@ static const struct address_space_operations shmem_aops = {
>> > .error_remove_page = generic_error_remove_page,
>> > };
>> >
>> > +/* Could arrive here for memfds opened through /proc/ */
>> > +int shmem_open(struct inode *inode, struct file *file)
>> > +{
>> > + struct shmem_inode_info *info = SHMEM_I(inode);
>> > +
>> > + /*
>> > + * memfds for which future writes have been prevented
>> > + * should not be reopened, say, through /proc/pid/fd/N
>> > + * symlinks otherwise it can cause a sealed memfd to be
>> > + * promoted to writable.
>> > + */
>> > + if (info->seals & F_SEAL_FUTURE_WRITE)
>> > + return -EACCES;
>> > +
>> > + return 0;
>> > +}
>>
>> The result of this series is very warty. We have a concept of seals, and they all work similarly, except the new future write seal. That one:
>>
>
> I don't see it as warty, different seals will work differently. It works
> quite well for our usecase, and since Linux is all about solving real
> problems in the real work, it would be useful to have it.
>
>> - causes a probably-observable effect in the file mode in F_GETFL.
>
> Wouldn't that be the right thing to observe anyway?
>
>> - causes reopen to fail.
>
> So this concern isn't true anymore if we make reopen fail only for WRITE
> opens as Daniel suggested. I will make this change so that the security fix
> is a clean one.
>
>> - does *not* affect other struct files that may already exist on the same inode.
>
> TBH if you really want to block all writes to the file, then you want
> F_SEAL_WRITE, not this seal. The usecase we have is the fd is sent over IPC
> to another process and we want to prevent any new writes in the receiver
> side. There is no way this other receiving process can have an existing fd
> unless it was already sent one without the seal applied. The proposed seal
> could be renamed to F_SEAL_FD_WRITE if that is preferred.
>
>> - mysteriously malfunctions if you try to set it again on another struct
>> file that already exists
>>
>
> I didn't follow this, could you explain more?
>
>> - probably is insecure when used on hugetlbfs.
>
> The usecase is not expected to prevent all writes, indeed the usecase
> requires existing mmaps to continue to be able to write into the memory map.
> So would you call that a security issue too? The use of the seal wants to
> allow existing mmap regions to be continue to be written into (I mentioned
> more details in the cover letter).
>
>> I see two reasonable solutions:
>>
>> 1. Don’t fiddle with the struct file at all. Instead make the inode flag
>> work by itself.
>
> Currently, the various VFS paths check only the struct file's f_mode to deny
> writes of already opened files. This would mean more checking in all those
> paths (and modification of all those paths).
>
> Anyway going with that idea, we could
> 1. call deny_write_access(file) from the memfd's seal path which decrements
> the inode::i_writecount.
> 2. call get_write_access(inode) in the various VFS paths in addition to
> checking for FMODE_*WRITE and deny the write (incase i_writecount is negative)
>
> That will prevent both reopens, and writes from succeeding. However I worry a
> bit about 2 not being too familiar with VFS internals, about what the
> consequences of doing that may be.
IMHO, modifying both the inode and the struct file separately is fine,
since they mean different things. In regular filesystems, it's fine to
have a read-write open file description for a file whose inode grants
write permission to nobody. Speaking of which: is fchmod enough to
prevent this attack?
>> 2. Don’t call it a “seal”. Instead fix the /proc hole and add an API to
>> drop write access on an existing struct file.
>>
>> I personally prefer #2.
>
> So I don't think proc has a hole looking at the code yesterday. As I was
> saying in other thread, its just how symlinks work. If we were to apply this
> API to files in general, and we had symlinks to files and we tried to reopen the
> file, we would run into the same issue - that's why I believe it has nothing
> to do with proc, and the fix has to be in the underlying inode being pointed
> to. Does that make sense or did I miss something?
+1. Another point to consider is that even if we did somehow fix the
"proc hole", any other means (perhaps in the future) of obtaining an
inode reference would then be insecure, perhaps silently. For example
--- is there a way to use open_by_handle_at(2) to open a memfd file?
^ permalink raw reply
* Official Linux system wrapper library?
From: Daniel Colascione @ 2018-11-10 18:52 UTC (permalink / raw)
To: linux-kernel; +Cc: Joel Fernandes, Linux API
Now that glibc is basically not adding any new system call wrappers,
how about publishing an "official" system call glue library as part of
the kernel distribution, along with the uapi headers? I don't think
it's reasonable to expect people to keep using syscall(__NR_XXX) for
all new functionality, especially as the system grows increasingly
sophisticated capabilities (like the new mount API, and hopefully the
new process API) outside the strictures of the POSIX process.
^ permalink raw reply
* Re: Official Linux system wrapper library?
From: Willy Tarreau @ 2018-11-10 19:01 UTC (permalink / raw)
To: Daniel Colascione; +Cc: linux-kernel, Joel Fernandes, Linux API
In-Reply-To: <CAKOZuesB4R=dCz4merWQN0FSCGrXmOgUUr4ienSbStBJguNv8g@mail.gmail.com>
On Sat, Nov 10, 2018 at 10:52:06AM -0800, Daniel Colascione wrote:
> Now that glibc is basically not adding any new system call wrappers,
> how about publishing an "official" system call glue library as part of
> the kernel distribution, along with the uapi headers? I don't think
> it's reasonable to expect people to keep using syscall(__NR_XXX) for
> all new functionality, especially as the system grows increasingly
> sophisticated capabilities (like the new mount API, and hopefully the
> new process API) outside the strictures of the POSIX process.
It's partly related, but you may be interested in something I did that
is in the the RCU tree. It's called "nolibc", it's a set of syscall
wrappers defined only in include files. It's not complete, but still
enough to boot some small init wrappers. Mine can extract tar files
and do stuff like this with it. Here is the kernel port in the RCU
tree and an example of code using it :
https://git.kernel.org/pub/scm/linux/kernel/git/paulmck/linux-rcu.git/tree/tools/testing/selftests/rcutorture/bin/nolibc.h?h=rcu/next
https://git.kernel.org/pub/scm/linux/kernel/git/paulmck/linux-rcu.git/tree/tools/testing/selftests/rcutorture/bin/mkinitrd.sh?h=rcu/next
The original one is maintained here (not very active since it works
well enough for my use cases now eventhough it's far from being
complete) :
http://git.formilux.org/?p=people/willy/nolibc.git
Maybe something along this could be done for the vast majority of
syscalls and the thicker stuff be left to glibc ? That would allow
simple userland to build without glibc using only kernel headers,
or by occasionally defining a few extra stuff or glue.
Willy
^ permalink raw reply
* Re: Official Linux system wrapper library?
From: Daniel Colascione @ 2018-11-10 19:06 UTC (permalink / raw)
To: Willy Tarreau; +Cc: linux-kernel, Joel Fernandes, Linux API
In-Reply-To: <20181110190132.GA21185@1wt.eu>
On Sat, Nov 10, 2018 at 11:01 AM, Willy Tarreau <w@1wt.eu> wrote:
> On Sat, Nov 10, 2018 at 10:52:06AM -0800, Daniel Colascione wrote:
>> Now that glibc is basically not adding any new system call wrappers,
>> how about publishing an "official" system call glue library as part of
>> the kernel distribution, along with the uapi headers? I don't think
>> it's reasonable to expect people to keep using syscall(__NR_XXX) for
>> all new functionality, especially as the system grows increasingly
>> sophisticated capabilities (like the new mount API, and hopefully the
>> new process API) outside the strictures of the POSIX process.
>
> It's partly related, but you may be interested in something I did that
> is in the the RCU tree. It's called "nolibc", it's a set of syscall
> wrappers defined only in include files. It's not complete, but still
> enough to boot some small init wrappers. Mine can extract tar files
> and do stuff like this with it. Here is the kernel port in the RCU
> tree and an example of code using it :
>
> https://git.kernel.org/pub/scm/linux/kernel/git/paulmck/linux-rcu.git/tree/tools/testing/selftests/rcutorture/bin/nolibc.h?h=rcu/next
> https://git.kernel.org/pub/scm/linux/kernel/git/paulmck/linux-rcu.git/tree/tools/testing/selftests/rcutorture/bin/mkinitrd.sh?h=rcu/next
>
> The original one is maintained here (not very active since it works
> well enough for my use cases now eventhough it's far from being
> complete) :
>
> http://git.formilux.org/?p=people/willy/nolibc.git
>
> Maybe something along this could be done for the vast majority of
> syscalls and the thicker stuff be left to glibc ? That would allow
> simple userland to build without glibc using only kernel headers,
> or by occasionally defining a few extra stuff or glue.
Reminds me of LSS: https://chromium.googlesource.com/linux-syscall-support/
I'm not a fan of this approach for general-purpose use. There's value
in having *some* common function-level indirection before actually
issuing system calls, e.g., for LD_PRELOAD stuff.
^ permalink raw reply
* Re: [PATCH v3 resend 1/2] mm: Add an F_SEAL_FUTURE_WRITE seal to memfd
From: Daniel Colascione @ 2018-11-10 19:11 UTC (permalink / raw)
To: Joel Fernandes
Cc: Andy Lutomirski, Jann Horn, kernel list, John Reck, John Stultz,
Todd Kjos, Greg Kroah-Hartman, Christoph Hellwig, Al Viro,
Andrew Morton, Bruce Fields, Jeff Layton, Khalid Aziz, Lei.Yang,
linux-fsdevel, linux-kselftest, Linux-MM, marcandre.lureau,
Mike Kravetz, Minchan Kim, Shuah Khan, Valdis
In-Reply-To: <CAKOZuesQXRtthJTEr86LByH3gPpAdT-PQM0d1jqr131=zZNRKw@mail.gmail.com>
On Sat, Nov 10, 2018 at 10:45 AM, Daniel Colascione <dancol@google.com> wrote:
> On Sat, Nov 10, 2018 at 10:24 AM, Joel Fernandes <joel@joelfernandes.org> wrote:
>> Thanks Andy for your thoughts, my comments below:
[snip]
>> I don't see it as warty, different seals will work differently. It works
>> quite well for our usecase, and since Linux is all about solving real
>> problems in the real work, it would be useful to have it.
>>
>>> - causes a probably-observable effect in the file mode in F_GETFL.
>>
>> Wouldn't that be the right thing to observe anyway?
>>
>>> - causes reopen to fail.
>>
>> So this concern isn't true anymore if we make reopen fail only for WRITE
>> opens as Daniel suggested. I will make this change so that the security fix
>> is a clean one.
>>
>>> - does *not* affect other struct files that may already exist on the same inode.
>>
>> TBH if you really want to block all writes to the file, then you want
>> F_SEAL_WRITE, not this seal. The usecase we have is the fd is sent over IPC
>> to another process and we want to prevent any new writes in the receiver
>> side. There is no way this other receiving process can have an existing fd
>> unless it was already sent one without the seal applied. The proposed seal
>> could be renamed to F_SEAL_FD_WRITE if that is preferred.
>>
>>> - mysteriously malfunctions if you try to set it again on another struct
>>> file that already exists
>>>
>>
>> I didn't follow this, could you explain more?
>>
>>> - probably is insecure when used on hugetlbfs.
>>
>> The usecase is not expected to prevent all writes, indeed the usecase
>> requires existing mmaps to continue to be able to write into the memory map.
>> So would you call that a security issue too? The use of the seal wants to
>> allow existing mmap regions to be continue to be written into (I mentioned
>> more details in the cover letter).
>>
>>> I see two reasonable solutions:
>>>
>>> 1. Don’t fiddle with the struct file at all. Instead make the inode flag
>>> work by itself.
>>
>> Currently, the various VFS paths check only the struct file's f_mode to deny
>> writes of already opened files. This would mean more checking in all those
>> paths (and modification of all those paths).
>>
>> Anyway going with that idea, we could
>> 1. call deny_write_access(file) from the memfd's seal path which decrements
>> the inode::i_writecount.
>> 2. call get_write_access(inode) in the various VFS paths in addition to
>> checking for FMODE_*WRITE and deny the write (incase i_writecount is negative)
>>
>> That will prevent both reopens, and writes from succeeding. However I worry a
>> bit about 2 not being too familiar with VFS internals, about what the
>> consequences of doing that may be.
>
> IMHO, modifying both the inode and the struct file separately is fine,
> since they mean different things. In regular filesystems, it's fine to
> have a read-write open file description for a file whose inode grants
> write permission to nobody. Speaking of which: is fchmod enough to
> prevent this attack?
Well, yes and no. fchmod does prevent reopening the file RW, but
anyone with permissions (owner, CAP_FOWNER) can just fchmod it back. A
seal is supposed to be irrevocable, so fchmod-as-inode-seal probably
isn't sufficient by itself. While it might be good enough for Android
(in the sense that it'll prevent RW-reopens from other security
contexts to which we send an open memfd file), it's still conceptually
ugly, IMHO. Let's go with the original approach of just tweaking the
inode so that open-for-write is permanently blocked.
^ permalink raw reply
* Re: Official Linux system wrapper library?
From: Greg KH @ 2018-11-10 19:20 UTC (permalink / raw)
To: Daniel Colascione; +Cc: linux-kernel, Joel Fernandes, Linux API
In-Reply-To: <CAKOZuesB4R=dCz4merWQN0FSCGrXmOgUUr4ienSbStBJguNv8g@mail.gmail.com>
On Sat, Nov 10, 2018 at 10:52:06AM -0800, Daniel Colascione wrote:
> Now that glibc is basically not adding any new system call wrappers,
Why are they not doing that anymore?
And there's no reason you have to use glibc, there are many other libcs
out there that hopefully are adding the new syscalls :)
> how about publishing an "official" system call glue library as part of
> the kernel distribution, along with the uapi headers? I don't think
> it's reasonable to expect people to keep using syscall(__NR_XXX) for
> all new functionality, especially as the system grows increasingly
> sophisticated capabilities (like the new mount API, and hopefully the
> new process API) outside the strictures of the POSIX process.
Patches are always welcome to be reviewed. But watch out that they
don't conflict with the libc headers. I know we had a "klibc" proposed
a long time ago but that died off for various reasons before it could
get merged.
Also, what about the basic work of making sure our uapi header files can
actually be used untouched by a libc? That isn't the case these days as
the bionic maintainers like to keep reminding me. That might be a good
thing to do _before_ trying to add new things like syscall wrappers.
thanks,
greg k-h
^ permalink raw reply
* Re: Official Linux system wrapper library?
From: Willy Tarreau @ 2018-11-10 19:33 UTC (permalink / raw)
To: Daniel Colascione; +Cc: linux-kernel, Joel Fernandes, Linux API
In-Reply-To: <CAKOZuevup11kuHGb3QD5GuA1c7_B=Nnq-kDEB5Hbv=OqXVotQQ@mail.gmail.com>
On Sat, Nov 10, 2018 at 11:06:45AM -0800, Daniel Colascione wrote:
> Reminds me of LSS: https://chromium.googlesource.com/linux-syscall-support/
Interesting, thanks for the link, I would probably not have started mine
had I known this one :-)
> I'm not a fan of this approach for general-purpose use. There's value
> in having *some* common function-level indirection before actually
> issuing system calls, e.g., for LD_PRELOAD stuff.
I'm not speaking about general purpose replacement but more about
general purpose low level functions that glibc wrappers can safely
use and expose by default. This way general purpose applications
would still use glibc and those willing to use a lower level could
do it more easily by accessing the lower layer, without having to
define their own syscalls. If I could do something like this in my
code :
#ifndef HAVE_SYSCALL_SPLICE // exposed by glibc
# ifdef __linux_splice // exposed by kernel header
# define splice __linux_splice
# else
# error "no splice exposed by either libc or kernel headers"
# endif
#endif
It would be easier, safer and cleaner than what I've used to do before :
#if !defined(HAVE_SYSCALL_SPLICE) && defined(__NR_splice)
static inline _syscall6(int, splice, int, fdin, loff_t *, off_in, int, fdout, loff_t *, off_out, size_t, len, unsigned long, flags);
#endif
Willy
^ permalink raw reply
* Re: [PATCH v3 resend 1/2] mm: Add an F_SEAL_FUTURE_WRITE seal to memfd
From: Andy Lutomirski @ 2018-11-10 19:55 UTC (permalink / raw)
To: Daniel Colascione
Cc: Joel Fernandes, Jann Horn, kernel list, John Reck, John Stultz,
Todd Kjos, Greg Kroah-Hartman, Christoph Hellwig, Al Viro,
Andrew Morton, Bruce Fields, Jeff Layton, Khalid Aziz, Lei.Yang,
linux-fsdevel, linux-kselftest, Linux-MM, marcandre.lureau,
Mike Kravetz, Minchan Kim, Shuah Khan, Valdis
In-Reply-To: <CAKOZueum8MtNvJ5P=W7_pRw62TdQdCgyjCwwbG1wezNboC1cxQ@mail.gmail.com>
> On Nov 10, 2018, at 11:11 AM, Daniel Colascione <dancol@google.com> wrote:
>
>> On Sat, Nov 10, 2018 at 10:45 AM, Daniel Colascione <dancol@google.com> wrote:
>>> On Sat, Nov 10, 2018 at 10:24 AM, Joel Fernandes <joel@joelfernandes.org> wrote:
>>> Thanks Andy for your thoughts, my comments below:
> [snip]
>>> I don't see it as warty, different seals will work differently. It works
>>> quite well for our usecase, and since Linux is all about solving real
>>> problems in the real work, it would be useful to have it.
>>>
>>>> - causes a probably-observable effect in the file mode in F_GETFL.
>>>
>>> Wouldn't that be the right thing to observe anyway?
>>>
>>>> - causes reopen to fail.
>>>
>>> So this concern isn't true anymore if we make reopen fail only for WRITE
>>> opens as Daniel suggested. I will make this change so that the security fix
>>> is a clean one.
>>>
>>>> - does *not* affect other struct files that may already exist on the same inode.
>>>
>>> TBH if you really want to block all writes to the file, then you want
>>> F_SEAL_WRITE, not this seal. The usecase we have is the fd is sent over IPC
>>> to another process and we want to prevent any new writes in the receiver
>>> side. There is no way this other receiving process can have an existing fd
>>> unless it was already sent one without the seal applied. The proposed seal
>>> could be renamed to F_SEAL_FD_WRITE if that is preferred.
>>>
>>>> - mysteriously malfunctions if you try to set it again on another struct
>>>> file that already exists
>>>>
>>>
>>> I didn't follow this, could you explain more?
>>>
>>>> - probably is insecure when used on hugetlbfs.
>>>
>>> The usecase is not expected to prevent all writes, indeed the usecase
>>> requires existing mmaps to continue to be able to write into the memory map.
>>> So would you call that a security issue too? The use of the seal wants to
>>> allow existing mmap regions to be continue to be written into (I mentioned
>>> more details in the cover letter).
>>>
>>>> I see two reasonable solutions:
>>>>
>>>> 1. Don’t fiddle with the struct file at all. Instead make the inode flag
>>>> work by itself.
>>>
>>> Currently, the various VFS paths check only the struct file's f_mode to deny
>>> writes of already opened files. This would mean more checking in all those
>>> paths (and modification of all those paths).
>>>
>>> Anyway going with that idea, we could
>>> 1. call deny_write_access(file) from the memfd's seal path which decrements
>>> the inode::i_writecount.
>>> 2. call get_write_access(inode) in the various VFS paths in addition to
>>> checking for FMODE_*WRITE and deny the write (incase i_writecount is negative)
>>>
>>> That will prevent both reopens, and writes from succeeding. However I worry a
>>> bit about 2 not being too familiar with VFS internals, about what the
>>> consequences of doing that may be.
>>
>> IMHO, modifying both the inode and the struct file separately is fine,
>> since they mean different things. In regular filesystems, it's fine to
>> have a read-write open file description for a file whose inode grants
>> write permission to nobody. Speaking of which: is fchmod enough to
>> prevent this attack?
>
> Well, yes and no. fchmod does prevent reopening the file RW, but
> anyone with permissions (owner, CAP_FOWNER) can just fchmod it back. A
> seal is supposed to be irrevocable, so fchmod-as-inode-seal probably
> isn't sufficient by itself. While it might be good enough for Android
> (in the sense that it'll prevent RW-reopens from other security
> contexts to which we send an open memfd file), it's still conceptually
> ugly, IMHO. Let's go with the original approach of just tweaking the
> inode so that open-for-write is permanently blocked.
This should be straightforward. Just add a new seal type and wire it up. It should be considerably simpler than SEAL_WRITE.
^ permalink raw reply
* Re: Official Linux system wrapper library?
From: Vlastimil Babka @ 2018-11-10 19:58 UTC (permalink / raw)
To: Greg KH, Daniel Colascione; +Cc: linux-kernel, Joel Fernandes, Linux API
In-Reply-To: <20181110192027.GA29892@kroah.com>
On 11/10/18 8:20 PM, Greg KH wrote:
> On Sat, Nov 10, 2018 at 10:52:06AM -0800, Daniel Colascione wrote:
>> Now that glibc is basically not adding any new system call wrappers,
>
> Why are they not doing that anymore?
FYI just noticed there's a topic relevant to this in LPC Toolchain MC:
https://linuxplumbersconf.org/event/2/contributions/149/
> And there's no reason you have to use glibc, there are many other libcs
> out there that hopefully are adding the new syscalls :)
>
>> how about publishing an "official" system call glue library as part of
>> the kernel distribution, along with the uapi headers? I don't think
>> it's reasonable to expect people to keep using syscall(__NR_XXX) for
>> all new functionality, especially as the system grows increasingly
>> sophisticated capabilities (like the new mount API, and hopefully the
>> new process API) outside the strictures of the POSIX process.
>
> Patches are always welcome to be reviewed. But watch out that they
> don't conflict with the libc headers. I know we had a "klibc" proposed
> a long time ago but that died off for various reasons before it could
> get merged.
>
> Also, what about the basic work of making sure our uapi header files can
> actually be used untouched by a libc? That isn't the case these days as
> the bionic maintainers like to keep reminding me. That might be a good
> thing to do _before_ trying to add new things like syscall wrappers.
>
> thanks,
>
> greg k-h
>
^ permalink raw reply
* Re: [PATCH v3 resend 1/2] mm: Add an F_SEAL_FUTURE_WRITE seal to memfd
From: Joel Fernandes @ 2018-11-10 22:09 UTC (permalink / raw)
To: Daniel Colascione
Cc: Andy Lutomirski, Jann Horn, kernel list, John Reck, John Stultz,
Todd Kjos, Greg Kroah-Hartman, Christoph Hellwig, Al Viro,
Andrew Morton, Bruce Fields, Jeff Layton, Khalid Aziz, Lei.Yang,
linux-fsdevel, linux-kselftest, Linux-MM, marcandre.lureau,
Mike Kravetz, Minchan Kim, Shuah Khan, Valdis
In-Reply-To: <CAKOZueum8MtNvJ5P=W7_pRw62TdQdCgyjCwwbG1wezNboC1cxQ@mail.gmail.com>
On Sat, Nov 10, 2018 at 11:11:27AM -0800, Daniel Colascione wrote:
> On Sat, Nov 10, 2018 at 10:45 AM, Daniel Colascione <dancol@google.com> wrote:
> > On Sat, Nov 10, 2018 at 10:24 AM, Joel Fernandes <joel@joelfernandes.org> wrote:
> >> Thanks Andy for your thoughts, my comments below:
> [snip]
> >> I don't see it as warty, different seals will work differently. It works
> >> quite well for our usecase, and since Linux is all about solving real
> >> problems in the real work, it would be useful to have it.
> >>
> >>> - causes a probably-observable effect in the file mode in F_GETFL.
> >>
> >> Wouldn't that be the right thing to observe anyway?
> >>
> >>> - causes reopen to fail.
> >>
> >> So this concern isn't true anymore if we make reopen fail only for WRITE
> >> opens as Daniel suggested. I will make this change so that the security fix
> >> is a clean one.
> >>
> >>> - does *not* affect other struct files that may already exist on the same inode.
> >>
> >> TBH if you really want to block all writes to the file, then you want
> >> F_SEAL_WRITE, not this seal. The usecase we have is the fd is sent over IPC
> >> to another process and we want to prevent any new writes in the receiver
> >> side. There is no way this other receiving process can have an existing fd
> >> unless it was already sent one without the seal applied. The proposed seal
> >> could be renamed to F_SEAL_FD_WRITE if that is preferred.
> >>
> >>> - mysteriously malfunctions if you try to set it again on another struct
> >>> file that already exists
> >>>
> >>
> >> I didn't follow this, could you explain more?
> >>
> >>> - probably is insecure when used on hugetlbfs.
> >>
> >> The usecase is not expected to prevent all writes, indeed the usecase
> >> requires existing mmaps to continue to be able to write into the memory map.
> >> So would you call that a security issue too? The use of the seal wants to
> >> allow existing mmap regions to be continue to be written into (I mentioned
> >> more details in the cover letter).
> >>
> >>> I see two reasonable solutions:
> >>>
> >>> 1. Don’t fiddle with the struct file at all. Instead make the inode flag
> >>> work by itself.
> >>
> >> Currently, the various VFS paths check only the struct file's f_mode to deny
> >> writes of already opened files. This would mean more checking in all those
> >> paths (and modification of all those paths).
> >>
> >> Anyway going with that idea, we could
> >> 1. call deny_write_access(file) from the memfd's seal path which decrements
> >> the inode::i_writecount.
> >> 2. call get_write_access(inode) in the various VFS paths in addition to
> >> checking for FMODE_*WRITE and deny the write (incase i_writecount is negative)
> >>
> >> That will prevent both reopens, and writes from succeeding. However I worry a
> >> bit about 2 not being too familiar with VFS internals, about what the
> >> consequences of doing that may be.
> >
> > IMHO, modifying both the inode and the struct file separately is fine,
> > since they mean different things. In regular filesystems, it's fine to
> > have a read-write open file description for a file whose inode grants
> > write permission to nobody. Speaking of which: is fchmod enough to
> > prevent this attack?
>
> Well, yes and no. fchmod does prevent reopening the file RW, but
> anyone with permissions (owner, CAP_FOWNER) can just fchmod it back. A
> seal is supposed to be irrevocable, so fchmod-as-inode-seal probably
> isn't sufficient by itself. While it might be good enough for Android
> (in the sense that it'll prevent RW-reopens from other security
> contexts to which we send an open memfd file), it's still conceptually
> ugly, IMHO. Let's go with the original approach of just tweaking the
> inode so that open-for-write is permanently blocked.
Agreed with the idea of modifying both file and inode flags. I was thinking
modifying i_mode may do the trick but as you pointed it probably could be
reverted by chmod or some other attribute setting calls.
OTOH, I don't think deny_write_access(file) can be reverted from any
user-facing path so we could do that from the seal to prevent the future
opens in write mode. I'll double check and test that out tomorrow.
thanks,
- Joel
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox