linux-kernel - Re: [PATCH v2 6/9] mm: Make hugetlb mappings go through mm_get_unmapped_area

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: <ZqtD4idp0IyHgSp5@localhost.localdomain>
Date: Thu, 1 Aug 2024 10:14:26 +0200
From: Oscar Salvador <osalvador@...e.de>
To: Lorenzo Stoakes <lorenzo.stoakes@...cle.com>
Cc: Andrew Morton <akpm@...ux-foundation.org>, linux-kernel@...r.kernel.org,
	linux-mm@...ck.org, Peter Xu <peterx@...hat.com>,
	Muchun Song <muchun.song@...ux.dev>,
	David Hildenbrand <david@...hat.com>,
	Donet Tom <donettom@...ux.ibm.com>,
	Matthew Wilcox <willy@...radead.org>,
	Vlastimil Babka <vbabka@...e.cz>, Michal Hocko <mhocko@...e.com>
Subject: Re: [PATCH v2 6/9] mm: Make hugetlb mappings go through
 mm_get_unmapped_area_vmflags

On Wed, Jul 31, 2024 at 05:15:41PM +0100, Lorenzo Stoakes wrote:
> I've not got the vm debug on in my build, so it's blowing up here for me:
> 
> static unsigned long shm_get_unmapped_area(struct file *file,
> 	unsigned long addr, unsigned long len, unsigned long pgoff,
> 	unsigned long flags)
> {
> 	struct shm_file_data *sfd = shm_file_data(file);
> 
> 	return sfd->file->f_op->get_unmapped_area(sfd->file, addr, len,
> 						pgoff, flags);
> }
> 
> Notice that that doesn't check whether sfd->file->f_op->get_unmapped_area
> is NULL.

I see now, thanks.

> So since you remove this from the f_ops, it causes a NULL pointer deref.
...
> static const struct file_operations shm_file_operations = {
> ..
> 	.get_unmapped_area	= shm_get_unmapped_area,
> ...
> };
> 
> Then this get_area() is invoked, which calls shm_get_unmapped_area(), which
> calls f_op->get_unmapped_area() on your hugetlbfs_file_operations object
> which you just deleted and it's NULL.
> 
> This is why you have to be super careful here, there's clearly stuff out
> there that assumes that this can't happen, which you need to track down.
> 
> A quick grep however _suggests_ this might be the one landmine place. But
> you need to find a smart way to deal with this.

Probably, the most straightforward way to fix this is to instead of
setting .get_unmapped_area to NULL for hugetlbfs_file_operations, would
be to have it re-defined like:

 .get_unmapped_area = mm_get_unmapped_area_vmflags

Which is what we call after this patchset.
So no more things have to tweaked.

On a more correct way, __maybe__ have something like:


 diff --git a/ipc/shm.c b/ipc/shm.c
 index 3e3071252dac..222dca8a3716 100644
 --- a/ipc/shm.c
 +++ b/ipc/shm.c
 @@ -648,8 +648,11 @@ static unsigned long shm_get_unmapped_area(struct file *file,
  {
  	struct shm_file_data *sfd = shm_file_data(file);
  
 -	return sfd->file->f_op->get_unmapped_area(sfd->file, addr, len,
 +	if (sfd->file->f_op->get_unmapped_area)
 +		return sfd->file->f_op->get_unmapped_area(sfd->file, addr, len,
  						pgoff, flags);
 +
 +	return mm_get_unmapped_area_vmflags(sfd->file, addr, len, pgoff, flags);
  }
  
  static const struct file_operations shm_file_operations = {

 
Still unsure about which approach looks more correct though.

-- 
Oscar Salvador
SUSE Labs