| Message ID | 20210120103436.11830-1-penguin-kernel@I-love.SAKURA.ne.jp (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] mm: memdup_user*() should use same gfp flags | expand |
On Wed, 20 Jan 2021 19:34:36 +0900 Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> 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]. > > 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 commit 6c2c97a24f096e32 ("memdup_user(): switch to GFP_USER"). That commit failed to explain why a switch to GFP_USER was performed, so that commit isn't a good substitute for an explanation of this change. So... please fully describe the reason for this change right here in this patch's changelog.
On 2021/01/22 10:35, Andrew Morton wrote: > On Wed, 20 Jan 2021 19:34:36 +0900 Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> 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]. >> >> 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 commit 6c2c97a24f096e32 ("memdup_user(): switch to GFP_USER"). > > That commit failed to explain why a switch to GFP_USER was performed, > so that commit isn't a good substitute for an explanation of this > change. For example, commit 2f77d107050abc14 ("Fix incorrect user space access locking in mincore()") silently converted GFP_KERNEL to GFP_USER. #define GFP_KERNEL (__GFP_RECLAIM | __GFP_IO | __GFP_FS) #define GFP_USER (__GFP_RECLAIM | __GFP_IO | __GFP_FS | __GFP_HARDWALL) * %GFP_KERNEL is typical for kernel-internal allocations. The caller requires * %ZONE_NORMAL or a lower zone for direct access but can direct reclaim. * %GFP_USER is for userspace allocations that also need to be directly * accessibly by the kernel or hardware. It is typically used by hardware * for buffers that are mapped to userspace (e.g. graphics) that hardware * still must DMA to. cpuset limits are enforced for these allocations. * %__GFP_HARDWALL enforces the cpuset memory allocation policy. > > So... please fully describe the reason for this change right here in > this patch's changelog. I guess that GFP_USER is chosen by cautious developers when memory is allocated by userspace request. Is there a guideline for when to use GFP_USER ?
On Fri 22-01-21 19:47:42, Tetsuo Handa wrote: > On 2021/01/22 10:35, Andrew Morton wrote: > > On Wed, 20 Jan 2021 19:34:36 +0900 Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> 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]. That is nasty! > >> 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 commit 6c2c97a24f096e32 ("memdup_user(): switch to GFP_USER"). No, this is papering over a more troubling underlying problem. Userspace shouldn't be able to trigger an aribitrary higher order allocations. Those users with a large size to copy should be really using kvmalloc based (e.g vmemdup_user). > > That commit failed to explain why a switch to GFP_USER was performed, > > so that commit isn't a good substitute for an explanation of this > > change. > > For example, commit 2f77d107050abc14 ("Fix incorrect user space access locking > in mincore()") silently converted GFP_KERNEL to GFP_USER. > > #define GFP_KERNEL (__GFP_RECLAIM | __GFP_IO | __GFP_FS) > #define GFP_USER (__GFP_RECLAIM | __GFP_IO | __GFP_FS | __GFP_HARDWALL) > > * %GFP_KERNEL is typical for kernel-internal allocations. The caller requires > * %ZONE_NORMAL or a lower zone for direct access but can direct reclaim. > > * %GFP_USER is for userspace allocations that also need to be directly > * accessibly by the kernel or hardware. It is typically used by hardware > * for buffers that are mapped to userspace (e.g. graphics) that hardware > * still must DMA to. cpuset limits are enforced for these allocations. > > * %__GFP_HARDWALL enforces the cpuset memory allocation policy. > > > > > So... please fully describe the reason for this change right here in > > this patch's changelog. > > I guess that GFP_USER is chosen by cautious developers when memory is > allocated by userspace request. Is there a guideline for when to use GFP_USER ? I do not think we have anything better than the above. GFP_USER is indeed used for userspace controlable allocations. So they can be a subject to a more strict cpu policy. memdup_user_nul looks like a good fit for GFP_USER to me. memdup_user and other variant already does this.
On 2021/01/25 22:32, Michal Hocko wrote: > On Fri 22-01-21 19:47:42, Tetsuo Handa wrote: >> On 2021/01/22 10:35, Andrew Morton wrote: >>> On Wed, 20 Jan 2021 19:34:36 +0900 Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> 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]. > > That is nasty! That's because -EFAULT will not be detected before memory allocation succeeds. Fuzzer is passing huge size without corresponding valid buffer. syscall(__NR_write, r[0], 0x200000c0ul, 0x200000cbul); > >>>> 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 commit 6c2c97a24f096e32 ("memdup_user(): switch to GFP_USER"). > > No, this is papering over a more troubling underlying problem. Userspace > shouldn't be able to trigger an aribitrary higher order allocations. That requires inserting max size checking before calling memdup_user_nul(). Oh, scattering around such checking is not nice. Add max length argument into memdup_user_nul() like strndup_user() ? > Those users with a large size to copy should be really using kvmalloc > based (e.g vmemdup_user). No. The caller in this case (writing an entry to smackfs) is not expecting such large allocations. Sane allocation size would be always less than PAGE_SIZE. > >>> That commit failed to explain why a switch to GFP_USER was performed, >>> so that commit isn't a good substitute for an explanation of this >>> change. >> >> For example, commit 2f77d107050abc14 ("Fix incorrect user space access locking >> in mincore()") silently converted GFP_KERNEL to GFP_USER. >> >> #define GFP_KERNEL (__GFP_RECLAIM | __GFP_IO | __GFP_FS) >> #define GFP_USER (__GFP_RECLAIM | __GFP_IO | __GFP_FS | __GFP_HARDWALL) >> >> * %GFP_KERNEL is typical for kernel-internal allocations. The caller requires >> * %ZONE_NORMAL or a lower zone for direct access but can direct reclaim. >> >> * %GFP_USER is for userspace allocations that also need to be directly >> * accessibly by the kernel or hardware. It is typically used by hardware >> * for buffers that are mapped to userspace (e.g. graphics) that hardware >> * still must DMA to. cpuset limits are enforced for these allocations. >> >> * %__GFP_HARDWALL enforces the cpuset memory allocation policy. >> >>> >>> So... please fully describe the reason for this change right here in >>> this patch's changelog. >> >> I guess that GFP_USER is chosen by cautious developers when memory is >> allocated by userspace request. Is there a guideline for when to use GFP_USER ? > > I do not think we have anything better than the above. GFP_USER is > indeed used for userspace controlable allocations. So they can be a > subject to a more strict cpu policy. memdup_user_nul looks like a good > fit for GFP_USER to me. memdup_user and other variant already does this. > Hmm, Sabyrzhan already proposed a patch that adds size check to the caller, but it seems that that patch missed smk_write_ambient()/smk_write_onlycap()/smk_write_unconfined() etc. Oh, bug-prone approach. Why not handle at memdup_user_nul() side?
On Mon 25-01-21 23:20:41, Tetsuo Handa wrote: > On 2021/01/25 22:32, Michal Hocko wrote: > > On Fri 22-01-21 19:47:42, Tetsuo Handa wrote: > >> On 2021/01/22 10:35, Andrew Morton wrote: > >>> On Wed, 20 Jan 2021 19:34:36 +0900 Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> 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]. > > > > That is nasty! > > That's because -EFAULT will not be detected before memory allocation succeeds. > Fuzzer is passing huge size without corresponding valid buffer. > > syscall(__NR_write, r[0], 0x200000c0ul, 0x200000cbul); > > > > >>>> 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 commit 6c2c97a24f096e32 ("memdup_user(): switch to GFP_USER"). > > > > No, this is papering over a more troubling underlying problem. Userspace > > shouldn't be able to trigger an aribitrary higher order allocations. > > That requires inserting max size checking before calling memdup_user_nul(). > Oh, scattering around such checking is not nice. Add max length argument > into memdup_user_nul() like strndup_user() ? Or simply fallback to the vmalloc based memdump* if the size is larger than the PAGE_SIZE. It seems that the existing API is much more complex than necessary. [...] > > I do not think we have anything better than the above. GFP_USER is > > indeed used for userspace controlable allocations. So they can be a > > subject to a more strict cpu policy. memdup_user_nul looks like a good > > fit for GFP_USER to me. memdup_user and other variant already does this. > > > > Hmm, Sabyrzhan already proposed a patch that adds size check to the caller, but it seems > that that patch missed smk_write_ambient()/smk_write_onlycap()/smk_write_unconfined() etc. > Oh, bug-prone approach. Why not handle at memdup_user_nul() side? I am sorry I do not follow.
> > Hmm, Sabyrzhan already proposed a patch that adds size check to the caller, but it seems > > that that patch missed smk_write_ambient()/smk_write_onlycap()/smk_write_unconfined() etc. > > Oh, bug-prone approach. Why not handle at memdup_user_nul() side? > I am sorry I do not follow. Tetsuo refers to this smackfs patch [1], where I've added a length check before memdup_user_nul(). There are currently 39 references to this function, where length > PAGE_SIZE - 1 or similar sanity check already presents. So I can't comment on handling it without __GFP_NOWARN at memdup_user_nul() side. > > Hmm, Sabyrzhan already proposed a patch that adds size check to the caller, but it seems > > that that patch missed smk_write_ambient()/smk_write_onlycap()/smk_write_unconfined() etc. Thanks, I will prepare PATCH v2 with a length check for smk_write_* smackfs functions in [1] patch set. [1] https://lore.kernel.org/linux-security-module/20210124143627.582115-1-snovitoll@gmail.com/
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]. 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 commit 6c2c97a24f096e32 ("memdup_user(): switch to GFP_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(-)