linux-kernel - Re: [PATCH v3] Fixes: null pointer dereference in pfnmap_lockdep

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] [day] [month] [year] [list]

Message-ID: <ZwPg4znfu2yn2Qqw@x1n>
Date: Mon, 7 Oct 2024 09:23:47 -0400
From: Peter Xu <peterx@...hat.com>
To: Matthew Wilcox <willy@...radead.org>
Cc: manas18244@...td.ac.in, Andrew Morton <akpm@...ux-foundation.org>,
	Shuah Khan <skhan@...uxfoundation.org>,
	Anup Sharma <anupnewsmail@...il.com>, linux-mm@...ck.org,
	linux-kernel@...r.kernel.org,
	syzbot+093d096417e7038a689b@...kaller.appspotmail.com
Subject: Re: [PATCH v3] Fixes: null pointer dereference in
 pfnmap_lockdep_assert

On Fri, Oct 04, 2024 at 04:17:42PM +0100, Matthew Wilcox wrote:
> On Fri, Oct 04, 2024 at 07:15:48PM +0530, Manas via B4 Relay wrote:
> > +++ b/mm/memory.c
> > @@ -6346,10 +6346,10 @@ static inline void pfnmap_args_setup(struct follow_pfnmap_args *args,
> >  static inline void pfnmap_lockdep_assert(struct vm_area_struct *vma)
> >  {
> >  #ifdef CONFIG_LOCKDEP
> > -	struct address_space *mapping = vma->vm_file->f_mapping;
> > +	struct address_space *mapping = vma->vm_file ? vma->vm_file->f_mapping : NULL;
> 
> Overly long and complex line.  Much simpler to write:
> 
> 	struct address_space *mapping = NULL;
> 
> 	if (vma->vm_file)
> 		mapping = vma->vm_file->f_mapping;
> 
> >  	if (mapping)
> > -		lockdep_assert(lockdep_is_held(&vma->vm_file->f_mapping->i_mmap_rwsem) ||
> > +		lockdep_assert(lockdep_is_held(&mapping->i_mmap_rwsem) ||
> >  			       lockdep_is_held(&vma->vm_mm->mmap_lock));
> >  	else
> >  		lockdep_assert(lockdep_is_held(&vma->vm_mm->mmap_lock));
> 
> This one should have been lockdep_assert_held(&vma->vm_mm->mmap_lock).
> 
> I'm not sure that the previous one is correct.  The
> lockdep_assert_held() macro is pretty careful about checking
> LOCK_STATE_NOT_HELD to avoid the LOCK_STATE_UNKNOWN possibility.
> But I'll leave that for Peter to fix.

Indeed..

Then looks like we could have quite a few other places in Linux that can
have used this wrong.. when the assert wants to check against either of the
two locks (one mutex or rcu read lock, for example) is held.

I'll send a patch after this one lands.

Thanks,

-- 
Peter Xu