| Message ID | 20210127105538.4919-1-penguin-kernel@I-love.SAKURA.ne.jp (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v3] mm: memdup_user*() should use same gfp flags | expand |
On Wed 27-01-21 19:55:38, Tetsuo Handa wrote: > syzbot is reporting that memdup_user_nul() which receives user-controlled > size (which can be up to (INT_MAX & PAGE_MASK)) via vfs_write() will hit > order >= MAX_ORDER path [1]. > > Making costly allocations (order > PAGE_ALLOC_COSTLY_ORDER) naturally fail > should be better than trying to enforce PAGE_SIZE upper limit, for some of > callers accept space-delimited list arguments. > > Therefore, let's add __GFP_NOWARN to memdup_user_nul() as with > commit 6c8fcc096be9d02f ("mm: don't let userspace spam allocations > warnings"). Also use GFP_USER as with other userspace-controllable > allocations like memdup_user(). I absolutely detest hiding this behind __GFP_NOWARN. There should be no reason to even try hard for memdup_user_nul. Can you explain why this cannot use kvmalloc instead? > [1] https://syzkaller.appspot.com/bug?id=8bf7efb3db19101b4008dc9198522ef977d098a6 > > Reported-by: syzbot <syzbot+a71a442385a0b2815497@syzkaller.appspotmail.com> > Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> > --- > mm/util.c | 7 +------ > 1 file changed, 1 insertion(+), 6 deletions(-) > > diff --git a/mm/util.c b/mm/util.c > index 8c9b7d1e7c49..265b40a86856 100644 > --- a/mm/util.c > +++ b/mm/util.c > @@ -252,12 +252,7 @@ void *memdup_user_nul(const void __user *src, size_t len) > { > char *p; > > - /* > - * Always use GFP_KERNEL, since copy_from_user() can sleep and > - * cause pagefault, which makes it pointless to use GFP_NOFS > - * or GFP_ATOMIC. > - */ > - p = kmalloc_track_caller(len + 1, GFP_KERNEL); > + p = kmalloc_track_caller(len + 1, GFP_USER | __GFP_NOWARN); > if (!p) > return ERR_PTR(-ENOMEM); > > -- > 2.18.4 >
On Wed 27-01-21 12:59:28, Michal Hocko wrote: > On Wed 27-01-21 19:55:38, Tetsuo Handa wrote: > > syzbot is reporting that memdup_user_nul() which receives user-controlled > > size (which can be up to (INT_MAX & PAGE_MASK)) via vfs_write() will hit > > order >= MAX_ORDER path [1]. > > > > Making costly allocations (order > PAGE_ALLOC_COSTLY_ORDER) naturally fail > > should be better than trying to enforce PAGE_SIZE upper limit, for some of > > callers accept space-delimited list arguments. > > > > Therefore, let's add __GFP_NOWARN to memdup_user_nul() as with > > commit 6c8fcc096be9d02f ("mm: don't let userspace spam allocations > > warnings"). Also use GFP_USER as with other userspace-controllable > > allocations like memdup_user(). > > I absolutely detest hiding this behind __GFP_NOWARN. There should be no > reason to even try hard for memdup_user_nul. Can you explain why this this should have been "try hard to get a physicaly contiguous memory for memdup_user_nul" > cannot use kvmalloc instead?
On Wed, 27 Jan 2021 23:03:33 +0900 Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp> wrote: > On 2021/01/27 21:17, Michal Hocko wrote: > > On Wed 27-01-21 12:59:28, Michal Hocko wrote: > >> On Wed 27-01-21 19:55:38, Tetsuo Handa wrote: > >>> syzbot is reporting that memdup_user_nul() which receives user-controlled > >>> size (which can be up to (INT_MAX & PAGE_MASK)) via vfs_write() will hit > >>> order >= MAX_ORDER path [1]. > >>> > >>> Making costly allocations (order > PAGE_ALLOC_COSTLY_ORDER) naturally fail > >>> should be better than trying to enforce PAGE_SIZE upper limit, for some of > >>> callers accept space-delimited list arguments. > >>> > >>> Therefore, let's add __GFP_NOWARN to memdup_user_nul() as with > >>> commit 6c8fcc096be9d02f ("mm: don't let userspace spam allocations > >>> warnings"). Also use GFP_USER as with other userspace-controllable > >>> allocations like memdup_user(). > >> > >> I absolutely detest hiding this behind __GFP_NOWARN. There should be no > >> reason to even try hard for memdup_user_nul. Can you explain why this > > > > this should have been "try hard to get a physicaly contiguous memory for memdup_user_nul" > > > >> cannot use kvmalloc instead? > > > > There is no point with allowing userspace to allocate 2GB of physically non-contiguous > memory using kvmalloc(). Size is controlled by userspace, and memdup_user_nul() is used > for allocating temporary memory which will be released before returning to userspace. > > Sane userspace processes should allocate only one or a few pages using memdup_user_nul(). > Just making insane user processes (like fuzzer) fail memory allocation requests is a > reasonable decision. (cc Casey) I'd say that the immediate problem is in smk_write_syslog(). Obviously it was implemented expecting small writes, but the fuzzer is passing it a huge write and things fall apart.
On 1/27/2021 3:19 PM, Andrew Morton wrote: > On Wed, 27 Jan 2021 23:03:33 +0900 Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp> wrote: > >> On 2021/01/27 21:17, Michal Hocko wrote: >>> On Wed 27-01-21 12:59:28, Michal Hocko wrote: >>>> On Wed 27-01-21 19:55:38, Tetsuo Handa wrote: >>>>> syzbot is reporting that memdup_user_nul() which receives user-controlled >>>>> size (which can be up to (INT_MAX & PAGE_MASK)) via vfs_write() will hit >>>>> order >= MAX_ORDER path [1]. >>>>> >>>>> Making costly allocations (order > PAGE_ALLOC_COSTLY_ORDER) naturally fail >>>>> should be better than trying to enforce PAGE_SIZE upper limit, for some of >>>>> callers accept space-delimited list arguments. >>>>> >>>>> Therefore, let's add __GFP_NOWARN to memdup_user_nul() as with >>>>> commit 6c8fcc096be9d02f ("mm: don't let userspace spam allocations >>>>> warnings"). Also use GFP_USER as with other userspace-controllable >>>>> allocations like memdup_user(). >>>> I absolutely detest hiding this behind __GFP_NOWARN. There should be no >>>> reason to even try hard for memdup_user_nul. Can you explain why this >>> this should have been "try hard to get a physicaly contiguous memory for memdup_user_nul" >>> >>>> cannot use kvmalloc instead? >> There is no point with allowing userspace to allocate 2GB of physically non-contiguous >> memory using kvmalloc(). Size is controlled by userspace, and memdup_user_nul() is used >> for allocating temporary memory which will be released before returning to userspace. >> >> Sane userspace processes should allocate only one or a few pages using memdup_user_nul(). >> Just making insane user processes (like fuzzer) fail memory allocation requests is a >> reasonable decision. > (cc Casey) > > I'd say that the immediate problem is in smk_write_syslog(). Obviously > it was implemented expecting small writes, but the fuzzer is passing it a > huge write and things fall apart. Yes, Smack should be checking that. Patch is in the works. I hates fuzzers.
On 2021/01/28 8:27, Casey Schaufler wrote: >>> There is no point with allowing userspace to allocate 2GB of physically non-contiguous >>> memory using kvmalloc(). Size is controlled by userspace, and memdup_user_nul() is used >>> for allocating temporary memory which will be released before returning to userspace. >>> >>> Sane userspace processes should allocate only one or a few pages using memdup_user_nul(). >>> Just making insane user processes (like fuzzer) fail memory allocation requests is a >>> reasonable decision. >> (cc Casey) >> >> I'd say that the immediate problem is in smk_write_syslog(). Obviously >> it was implemented expecting small writes, but the fuzzer is passing it a >> huge write and things fall apart. > > Yes, Smack should be checking that. Patch is in the works. Caller of memdup_user_nul() is responsible for making sure that size != -1 in order to avoid integer overflow overlooked by kmalloc(0) != NULL because memdup_user_nul() allocates size + 1 bytes. And this is automatically made sure for smackfs because vfs_write() makes sure that size <= (INT_MAX & PAGE_MASK) bytes. But some legitimate userspace might be already doing "write(fd, buffer, 20000);" for smk_write_onlycap()/smk_write_relabel_self(). How can you guarantee that introducing upper limit on the caller side does not break existing userspace tools? If some caller wants size > 32767 for memdup_user_nul(), it is just a matter of introducing vmemdup_user_nul(). memdup_user() and memdup_user_nul() had better behave similarly. There is no reason to use different gfp flags between memdup_user() and memdup_user_nul(). > I hates fuzzers. A surprising comment from security person. Smack is free to opt out of syzbot testing. :-)
On Wed 27-01-21 15:19:40, Andrew Morton wrote: > On Wed, 27 Jan 2021 23:03:33 +0900 Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp> wrote: > > > On 2021/01/27 21:17, Michal Hocko wrote: > > > On Wed 27-01-21 12:59:28, Michal Hocko wrote: > > >> On Wed 27-01-21 19:55:38, Tetsuo Handa wrote: > > >>> syzbot is reporting that memdup_user_nul() which receives user-controlled > > >>> size (which can be up to (INT_MAX & PAGE_MASK)) via vfs_write() will hit > > >>> order >= MAX_ORDER path [1]. > > >>> > > >>> Making costly allocations (order > PAGE_ALLOC_COSTLY_ORDER) naturally fail > > >>> should be better than trying to enforce PAGE_SIZE upper limit, for some of > > >>> callers accept space-delimited list arguments. > > >>> > > >>> Therefore, let's add __GFP_NOWARN to memdup_user_nul() as with > > >>> commit 6c8fcc096be9d02f ("mm: don't let userspace spam allocations > > >>> warnings"). Also use GFP_USER as with other userspace-controllable > > >>> allocations like memdup_user(). > > >> > > >> I absolutely detest hiding this behind __GFP_NOWARN. There should be no > > >> reason to even try hard for memdup_user_nul. Can you explain why this > > > > > > this should have been "try hard to get a physicaly contiguous memory for memdup_user_nul" > > > > > >> cannot use kvmalloc instead? > > > > > > > There is no point with allowing userspace to allocate 2GB of physically non-contiguous > > memory using kvmalloc(). Size is controlled by userspace, and memdup_user_nul() is used > > for allocating temporary memory which will be released before returning to userspace. > > > > Sane userspace processes should allocate only one or a few pages using memdup_user_nul(). > > Just making insane user processes (like fuzzer) fail memory allocation requests is a > > reasonable decision. > > (cc Casey) > > I'd say that the immediate problem is in smk_write_syslog(). Obviously > it was implemented expecting small writes, but the fuzzer is passing it a > huge write and things fall apart. I am not familiar with this particular caller and having a limit check which suits that particular usage is a reasonable thing to do. I do argue two things - using NOWARN to work around potentially buggy callers is just sweeping the mess under the rug and opens - these helper functions are to help copy user input and that doesn't really need physically contiguous pages. This can even become dangerous as a higher order depleting vector and DoS via OOM in the worst case. From that it sounds natural that the helper should be using kvmalloc. This will not solve a due size check on the caller side but that is not possible from a generic helper library function anyway. But it will provide a reasonable allocation policy.
diff --git a/mm/util.c b/mm/util.c index 8c9b7d1e7c49..265b40a86856 100644 --- a/mm/util.c +++ b/mm/util.c @@ -252,12 +252,7 @@ void *memdup_user_nul(const void __user *src, size_t len) { char *p; - /* - * Always use GFP_KERNEL, since copy_from_user() can sleep and - * cause pagefault, which makes it pointless to use GFP_NOFS - * or GFP_ATOMIC. - */ - p = kmalloc_track_caller(len + 1, GFP_KERNEL); + p = kmalloc_track_caller(len + 1, GFP_USER | __GFP_NOWARN); if (!p) return ERR_PTR(-ENOMEM);
syzbot is reporting that memdup_user_nul() which receives user-controlled size (which can be up to (INT_MAX & PAGE_MASK)) via vfs_write() will hit order >= MAX_ORDER path [1]. Making costly allocations (order > PAGE_ALLOC_COSTLY_ORDER) naturally fail should be better than trying to enforce PAGE_SIZE upper limit, for some of callers accept space-delimited list arguments. Therefore, let's add __GFP_NOWARN to memdup_user_nul() as with commit 6c8fcc096be9d02f ("mm: don't let userspace spam allocations warnings"). Also use GFP_USER as with other userspace-controllable allocations like memdup_user(). [1] https://syzkaller.appspot.com/bug?id=8bf7efb3db19101b4008dc9198522ef977d098a6 Reported-by: syzbot <syzbot+a71a442385a0b2815497@syzkaller.appspotmail.com> Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> --- mm/util.c | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-)