| Message ID | 20211001215630.810592-1-eric.dumazet@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] mm/mempolicy: do not allow illegal MPOL_F_NUMA_BALANCING | MPOL_LOCAL in mbind() | expand |
On Fri, 1 Oct 2021 14:56:30 -0700 Eric Dumazet <eric.dumazet@gmail.com> wrote: > From: Eric Dumazet <edumazet@google.com> > > syzbot reported access to unitialized memory in mbind() [1] I'm lazy. What memory is being accessed-unintialized? > Issue came with commit bda420b98505 ("numa balancing: migrate on > fault among multiple bound nodes") No cc:stable? What's the worst-case user-visible impact here?
On Fri, Oct 1, 2021 at 3:49 PM Andrew Morton <akpm@linux-foundation.org> wrote: > > On Fri, 1 Oct 2021 14:56:30 -0700 Eric Dumazet <eric.dumazet@gmail.com> wrote: > > > From: Eric Dumazet <edumazet@google.com> > > > > syzbot reported access to unitialized memory in mbind() [1] > > I'm lazy. What memory is being accessed-unintialized? I think you can clearly see that with this debug patch (courtesy of Alexander Potapenko) : (Then issue various mbind( ...MPOL_F_NUMA_BALANCING | MPOL_LOCAL ...) in a loop... ) diff --git a/mm/mempolicy.c b/mm/mempolicy.c index 1592b081c58e..95a4d71efe99 100644 --- a/mm/mempolicy.c +++ b/mm/mempolicy.c @@ -291,6 +291,7 @@ static struct mempolicy *mpol_new(unsigned short mode, unsigned short flags, } else if (nodes_empty(*nodes)) return ERR_PTR(-EINVAL); policy = kmem_cache_alloc(policy_cache, GFP_KERNEL); + memset(policy, 0xAA, sizeof(struct mempolicy)); if (!policy) return ERR_PTR(-ENOMEM); atomic_set(&policy->refcnt, 1); @@ -2256,9 +2257,12 @@ bool __mpol_equal(struct mempolicy *a, struct mempolicy *b) return false; if (a->flags != b->flags) return false; - if (mpol_store_user_nodemask(a)) + if (mpol_store_user_nodemask(a)) { + pr_err("struct mempolicy *a: %px, nodemask: %px\n", a, *(void**)&(a->w.user_nodemask)); + pr_err("struct mempolicy *b: %px, nodemask: %px\n", b, *(void**)&(b->w.user_nodemask)); if (!nodes_equal(a->w.user_nodemask, b->w.user_nodemask)) return false; + } switch (a->mode) { case MPOL_BIND: > > > Issue came with commit bda420b98505 ("numa balancing: migrate on > > fault among multiple bound nodes") > > No cc:stable? What's the worst-case user-visible impact here? I added the more precise tag : Fixes: bda420b98505 ("numa balancing: migrate on fault among multiple bound nodes") I only put Fixes: tag, so that stable teams can use their automation just fine. worst-case impact, I am not sure if any application ever used this undocumented combinations of flags ? Also, it is generally advised that accessing garbage values has undocumented behavior. A host could for example crash (it certainly does with KMSAN)
On Fri, Oct 01, 2021 at 04:37:40PM -0700, Eric Dumazet wrote: > > > Issue came with commit bda420b98505 ("numa balancing: migrate on > > > fault among multiple bound nodes") > > > > No cc:stable? What's the worst-case user-visible impact here? > > I added the more precise tag : Fixes: bda420b98505 ("numa balancing: > migrate on fault among multiple bound nodes") > I only put Fixes: tag, so that stable teams can use their automation just fine. > > worst-case impact, I am not sure if any application ever used this > undocumented combinations of flags ? > Also, it is generally advised that accessing garbage values has > undocumented behavior. > A host could for example crash (it certainly does with KMSAN) mm has special stable rules; fixes only get backported if explicitly requested instead of automatically like most of the rest of the kernel.
Hi, Eric, Eric Dumazet <eric.dumazet@gmail.com> writes: > From: Eric Dumazet <edumazet@google.com> > > syzbot reported access to unitialized memory in mbind() [1] > > Issue came with commit bda420b98505 ("numa balancing: migrate on > fault among multiple bound nodes") > > This commit added a new bit in MPOL_MODE_FLAGS, but only checked > valid combination (MPOL_F_NUMA_BALANCING can only be used with MPOL_BIND) > in do_set_mempolicy() > > This patch moves the check in sanitize_mpol_flags() so that it > is also used by mbind() Good catch! Thanks! When MPOL_F_NUMA_BALANCING is introduced, it is intended to be used with set_memopolicy() syscall only, it is not allowed to be used with mbind() syscall at least for now. But I misunderstood the original code apparently. So I think it may be better to return EINVAL for mbind() + MPOL_F_NUMA_BALANCING? Best Regards, Huang, Ying
diff --git a/mm/mempolicy.c b/mm/mempolicy.c index 1592b081c58ef6dd63c6f075ad24722f2be7cb5d..d12e0608fced235dc9137d0628437046299c7cfc 100644 --- a/mm/mempolicy.c +++ b/mm/mempolicy.c @@ -856,16 +856,6 @@ static long do_set_mempolicy(unsigned short mode, unsigned short flags, goto out; } - if (flags & MPOL_F_NUMA_BALANCING) { - if (new && new->mode == MPOL_BIND) { - new->flags |= (MPOL_F_MOF | MPOL_F_MORON); - } else { - ret = -EINVAL; - mpol_put(new); - goto out; - } - } - ret = mpol_set_nodemask(new, nodes, scratch); if (ret) { mpol_put(new); @@ -1458,7 +1448,11 @@ static inline int sanitize_mpol_flags(int *mode, unsigned short *flags) return -EINVAL; if ((*flags & MPOL_F_STATIC_NODES) && (*flags & MPOL_F_RELATIVE_NODES)) return -EINVAL; - + if (*flags & MPOL_F_NUMA_BALANCING) { + if (*mode != MPOL_BIND) + return -EINVAL; + *flags |= (MPOL_F_MOF | MPOL_F_MORON); + } return 0; }