linux-kernel - Re: [PATCH v3] x86/mm/pat: Fix VM_PAT handling when fork() fails in copy_page

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: <94c35e89-f915-4122-b1a0-436893201373@stanley.mountain>
Date: Thu, 3 Apr 2025 18:14:11 +0300
From: Dan Carpenter <dan.carpenter@...aro.org>
To: Lorenzo Stoakes <lorenzo.stoakes@...cle.com>
Cc: David Hildenbrand <david@...hat.com>, kernel test robot <lkp@...el.com>,
	oe-kbuild@...ts.linux.dev, Dan Carpenter <error27@...il.com>,
	linux-mm@...ck.org, linux-kernel@...r.kernel.org,
	"Liam R. Howlett" <Liam.Howlett@...cle.com>, x86@...nel.org
Subject: Re: [PATCH v3] x86/mm/pat: Fix VM_PAT handling when fork() fails in
 copy_page_range()

Sorry, I've been having trouble with my email recently...  I replied
earlier but my email got eaten on the way out.

What happened here is that the zero day bot emails go to me first and
then I review them or forward them depending on if they're a real
issue or not.

Here it's a false postive because it's set and used if the
(src_vma->vm_flags & VM_PFNMAP) flag is set.  Smatch doesn't parse
this correctly.  I've been meaning to fix this in Smatch for a
while.

But these days more and more people are using lei and b4 to get their
email so they see the unreviewed Smatch warnings.

regards,
dan carpenter

On Wed, Apr 02, 2025 at 12:37:24PM +0100, Lorenzo Stoakes wrote:
> Bcc:
> Subject: Re: [PATCH v3] x86/mm/pat: Fix VM_PAT handling when fork() fails in
>  copy_page_range()
> Message-ID: <0f94adaf-37a4-4d38-b952-01c2dc474a2c@...ifer.local>
> Reply-To:
> In-Reply-To: <b21bcd61-faf0-4ad8-b644-99794794594f@...hat.com>
> 
> Actually let me +cc a few more so this isn't lost further :P
> 
> On Wed, Apr 02, 2025 at 01:32:52PM +0200, David Hildenbrand wrote:
> > On 02.04.25 13:19, Lorenzo Stoakes wrote:
> > > On Thu, Mar 27, 2025 at 09:59:02AM +0800, kernel test robot wrote:
> > > > BCC: lkp@...el.com
> > > > CC: oe-kbuild-all@...ts.linux.dev
> > > > In-Reply-To: <20250325191951.471185-1-david@...hat.com>
> > > > References: <20250325191951.471185-1-david@...hat.com>
> > > > TO: David Hildenbrand <david@...hat.com>
> > > >
> > > > Hi David,
> > > >
> > > > kernel test robot noticed the following build warnings:
> > > >
> > > > [auto build test WARNING on 38fec10eb60d687e30c8c6b5420d86e8149f7557]
> > > >
> > > > url:    https://github.com/intel-lab-lkp/linux/commits/David-Hildenbrand/x86-mm-pat-Fix-VM_PAT-handling-when-fork-fails-in-copy_page_range/20250326-032200
> > > > base:   38fec10eb60d687e30c8c6b5420d86e8149f7557
> > > > patch link:    https://lore.kernel.org/r/20250325191951.471185-1-david%40redhat.com
> > > > patch subject: [PATCH v3] x86/mm/pat: Fix VM_PAT handling when fork() fails in copy_page_range()
> > > > :::::: branch date: 31 hours ago
> > > > :::::: commit date: 31 hours ago
> > > > config: hexagon-randconfig-r073-20250327 (https://download.01.org/0day-ci/archive/20250327/202503270941.IFILyNCX-lkp@intel.com/config)
> > > > compiler: clang version 21.0.0git (https://github.com/llvm/llvm-project c2692afc0a92cd5da140dfcdfff7818a5b8ce997)
> > > >
> > > > If you fix the issue in a separate patch/commit (i.e. not just a new version of
> > > > the same patch/commit), kindly add following tags
> > > > | Reported-by: kernel test robot <lkp@...el.com>
> > > > | Reported-by: Dan Carpenter <error27@...il.com>
> > > > | Closes: https://lore.kernel.org/r/202503270941.IFILyNCX-lkp@intel.com/
> > > >
> > > > smatch warnings:
> > > > mm/memory.c:1428 copy_page_range() error: uninitialized symbol 'pfn'.
> >
> > Huh,
> >
> > how did the original report not make it into my inbox ? :/
> 
> Yeah it's odd... maybe broken script?
> 
> >
> > Thanks for replying Lorenzo!
> 
> NP!
> 
> >
> > >
> > > I have a feeling this is because if ndef __HAVE_PFNMAP_TRACKING you just
> > > don't touch pfn at all, but also I see in the new track_pfn_copy() there
> > > are code paths where pfn doesn't get set, but you still pass the
> > > uninitialised pfn to untrack_pfn_copy()...
> >
> > If track_pfn_copy() returns 0 and VM_PAT applies, the pfn is set. Otherwise
> > (returns an error), we immediately return from copy_page_range().
> >
> > So once we reach untrack_pfn_copy() ... the PFN was set.
> >
> > In case of !__HAVE_PFNMAP_TRACKING the pfn is not set and not used.
> >
> > >
> > > I mean it could also be in the case of !(src_vma->vm_flags & VM_PAT) (but &
> > > VM_PFNMAP), where we return 0 but still pass pfn to untrack_pfn_copy()...
> >
> > I assume that's what it is complaining about, and it doesn't figure out that
> > the parameter is unused.
> >
> > So likely it's best to just initialize pfn to 0.
> >
> > >
> > > This is all super icky, we probably want to actually have track_pfn_copy()
> > > indicate whether we want to later untrack, not only if there's an error.
> >
> > Sounds overly-complicated. But having a pfn != 0 might work.
> >
> > > > Will comment accordingly on patch, but I mean I don't like the idea of
> > us
> > > just initialising the pfn here, because... what to?... :)
> >
> 
> Sure, I mean for all of above let's have the debate on the main patch I guess so
> it's in one place...
> 
> > Stared at that code for too long (and I reached a point where the PAT stuff
> > absolutely annoys me).
> 
> But, also lol. Can. Relate.
> 
> >
> > Thanks!
> >
> > --
> > Cheers,
> >
> > David / dhildenb
> >
> 
> Cheers, Lorenzo