linux-kernel - Re: [PATCH] mm/zswap: fix error pointer free in zswap_cpu_comp

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 for Android: free password hash cracker in your pocket

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

Message-ID: <20260102143945.52356-1-sj@kernel.org>
Date: Fri,  2 Jan 2026 06:39:44 -0800
From: SeongJae Park <sj@...nel.org>
To: Pavel Butsykin <pbutsykin@...udlinux.com>
Cc: SeongJae Park <sj@...nel.org>,
	hannes@...xchg.org,
	yosry.ahmed@...ux.dev,
	nphamcs@...il.com,
	chengming.zhou@...ux.dev,
	akpm@...ux-foundation.org,
	linux-mm@...ck.org,
	linux-kernel@...r.kernel.org,
	stable@...r.kernel.org
Subject: Re: [PATCH] mm/zswap: fix error pointer free in zswap_cpu_comp_prepare()

On Fri, 2 Jan 2026 10:51:01 +0400 Pavel Butsykin <pbutsykin@...udlinux.com> wrote:

> On 1/1/26 04:32, SeongJae Park wrote:
> > On Wed, 31 Dec 2025 11:46:38 +0400 Pavel Butsykin <pbutsykin@...udlinux.com> wrote:
> > 
> >> crypto_alloc_acomp_node() may return ERR_PTR(), but the fail path checks
> >> only for NULL and can pass an error pointer to crypto_free_acomp().
> >> Use IS_ERR_OR_NULL() to only free valid acomp instances.
> >>
> >> Fixes: 779b9955f643 ("mm: zswap: move allocations during CPU init outside the lock")
> >> Cc: stable@...r.kernel.org
> >> Signed-off-by: Pavel Butsykin <pbutsykin@...udlinux.com>
> >> ---
> >>   mm/zswap.c | 2 +-
> >>   1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/mm/zswap.c b/mm/zswap.c
> >> index 5d0f8b13a958..ac9b7a60736b 100644
> >> --- a/mm/zswap.c
> >> +++ b/mm/zswap.c
> >> @@ -787,7 +787,7 @@ static int zswap_cpu_comp_prepare(unsigned int cpu, struct hlist_node *node)
> >>   	return 0;
> >>   
> >>   fail:
> >> -	if (acomp)
> >> +	if (!IS_ERR_OR_NULL(acomp))
> >>   		crypto_free_acomp(acomp);
> >>   	kfree(buffer);
> >>   	return ret;
> > 
> > I understand you are keeping NULL case to keep the old behavior.  But, seems
> > the case cannot happen to me for following reasons.
> > 
> > First of all, the old NULL check was only for crypto_alloc_acomp_node()
> > failure.  But crypto_alloc_acomp_node() seems not returning NULL, to by breif
> > look of the code.  And the failure check of crypto_alloc_acomp_node() is
> > actually doing only IS_ERR() check.
> > 
> > So, it seems IS_ERR() here is enough.  Or, if I missed a case that
> > crypto_alloc_acomp_node() returns NULL, the above crypto_alloc_acomp_node()
> > failure check should be updated to use IS_ERR_OR_NULL()?
> > 
> 
> We have 'goto fail;' right before crypto_alloc_acomp_node() for the case 
> where kmalloc_node fails to allocate memory. In that case, 'acomp' will 
> still be initialized to NULL.

Ah, you are right.  Thank you for fixing this.  Please feel free to add

Reviewed-by: SeongJae Park <sj@...nel.org>


Thanks,
SJ

[...]