linux-kernel - Re: [PATCH] mm: kmem: properly initialize local objcg variable in current_obj

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 16 Nov 2023 15:56:27 +0100
From:   Erhard Furtner <erhard_f@...lbox.org>
To:     Vlastimil Babka <vbabka@...e.cz>
Cc:     Roman Gushchin <roman.gushchin@...ux.dev>,
        Andrew Morton <akpm@...ux-foundation.org>,
        linux-kernel@...r.kernel.org, Shakeel Butt <shakeelb@...gle.com>,
        David Rientjes <rientjes@...gle.com>,
        Dennis Zhou <dennis@...nel.org>,
        Johannes Weiner <hannes@...xchg.org>,
        Michal Hocko <mhocko@...nel.org>,
        Muchun Song <muchun.song@...ux.dev>, stable@...r.kernel.org
Subject: Re: [PATCH] mm: kmem: properly initialize local objcg variable in
 current_obj_cgroup()

On Thu, 16 Nov 2023 08:04:18 +0100
Vlastimil Babka <vbabka@...e.cz> wrote:

> On 11/16/23 03:51, Roman Gushchin wrote:
> > Actually the problem is caused by uninitialized local variable in
> > current_obj_cgroup(). If the root memory cgroup is set as an active
> > memory cgroup for a charging scope (as in the trace, where systemd
> > tries to create the first non-root cgroup, so the parent cgroup is
> > the root cgroup), the "for" loop is skipped and uninitialized objcg is
> > returned, causing a panic down the accounting stack.
> > 
> > The fix is trivial: initialize the objcg variable to NULL
> > unconditionally before the "for" loop.
> > 
> > Fixes: e86828e5446d ("mm: kmem: scoped objcg protection")
> > Reported-by: Erhard Furtner <erhard_f@...lbox.org>
> > Closes: https://github.com/ClangBuiltLinux/linux/issues/1959
> > Signed-off-by: Roman Gushchin (Cruise) <roman.gushchin@...ux.dev>
> > Cc: Shakeel Butt <shakeelb@...gle.com>
> > Cc: Vlastimil Babka <vbabka@...e.cz>
> > Cc: David Rientjes <rientjes@...gle.com>
> > Cc: Dennis Zhou <dennis@...nel.org>
> > Cc: Johannes Weiner <hannes@...xchg.org>
> > Cc: Michal Hocko <mhocko@...nel.org>
> > Cc: Muchun Song <muchun.song@...ux.dev>
> > Cc: stable@...r.kernel.org  
> 
> Acked-by: Vlastimil Babka <vbabka@...e.cz>
> 
> We could also do this to make it less confusing?
> 
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index 774bd6e21e27..a08bcec661b6 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -3175,7 +3175,6 @@ __always_inline struct obj_cgroup *current_obj_cgroup(void)
>  		objcg = rcu_dereference_check(memcg->objcg, 1);
>  		if (likely(objcg))
>  			break;
> -		objcg = NULL;
>  	}
>  
>  	return objcg;
> 
> 

I can confirm the 1st patch from Roman fixes the issue on my amd64 and on my i686 box.

The 2nd patch from Vlastimil unfortunately does not (only tried on amd64).

Regards,
Erhard