linux-kernel - Re: 回复: [PATCH] mm: Fix possible NULL pointer dereference in __swap

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]

Message-ID: <Z66B0T125JhdWo1n@google.com>
Date: Thu, 13 Feb 2025 23:35:45 +0000
From: Yosry Ahmed <yosry.ahmed@...ux.dev>
To: gaoxu <gaoxu2@...or.com>
Cc: Andrew Morton <akpm@...ux-foundation.org>,
	"linux-mm@...ck.org" <linux-mm@...ck.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	Suren Baghdasaryan <surenb@...gle.com>,
	Barry Song <21cnbao@...il.com>, yipengxiang <yipengxiang@...or.com>,
	Hugh Dickins <hughd@...gle.com>, Chris Li <chrisl@...nel.org>
Subject: Re: 回复: [PATCH] mm: Fix
 possible NULL pointer dereference in __swap_duplicate

On Thu, Feb 13, 2025 at 01:08:54PM +0000, gaoxu wrote:
> > 
> > On Wed, 12 Feb 2025 03:13:46 +0000 gaoxu <gaoxu2@...or.com> wrote:
> > 
> > > swp_swap_info() may return null; it is necessary to check the return
> > > value to avoid NULL pointer dereference. The code for other calls to
> > > swp_swap_info() includes checks, and __swap_duplicate() should also
> > > include checks.
> > 
> > Actually very few of the swp_swap_info() callers check for a NULL return.
> The swapfile.c file contains three instances where the return value of
> swp_swap_info() is checked for a NULL return. In other files that call
> swp_swap_info(), I have confirmed that there are no such checks.
> The description in the patch is inaccurate, and I have made modifications
> in patch v2.
> > 
> > > The reason why swp_swap_info() returns NULL is unclear; it may be due
> > > to CPU cache issues or DDR bit flips.
> > 
> > Quite possibly it's a kernel bug.
> > 
> > > The probability of this issue is very
> > > small, and the stack info we encountered is as follows：
> > > Unable to handle kernel NULL pointer dereference at virtual address
> > > 0000000000000058
> > >
> > > ...
> > >
> > > --- a/mm/swapfile.c
> > > +++ b/mm/swapfile.c
> > > @@ -3521,6 +3521,8 @@ static int __swap_duplicate(swp_entry_t entry,
> > unsigned char usage, int nr)
> > >  	int err, i;
> > >
> > >  	si = swp_swap_info(entry);
> > > +	if (unlikely(!si))
> > > +		return -EINVAL;
> > >
> > >  	offset = swp_offset(entry);
> > >  	VM_WARN_ON(nr > SWAPFILE_CLUSTER - offset % SWAPFILE_CLUSTER);
> > 
> > OK, I guess avoiding the crash is good.  But please let's include a WARN so that
> > we can perhaps fix the bug, if one is there.
> Good. I'll change it as mentioned and send a new patch.
> 	si = swp_swap_info(entry);
> +	if (unlikely(!si)) {
> +		WARN(1, KERN_ERR "%s: %s%08lx\n", __func__, Bad_file, entry.val);

WARN() already contains unlikely(). Also, no need to print the function
name it's already in the stack trace.

We should probably just do if (WARN_ON_ONCE(!si)).

> +		return -EINVAL;
> +	}
> 
>