linux-kernel - Re: [PATCH] proc: nommu: /proc/<pid>/maps: release mmap read lock

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

Message-ID: <ZQR76GSNEmG6w2oe@dell-precision-5540>
Date:   Fri, 15 Sep 2023 11:44:40 -0400
From:   Ben Wolsieffer <ben.wolsieffer@...ring.com>
To:     Oleg Nesterov <oleg@...hat.com>
Cc:     linux-kernel@...r.kernel.org, linux-fsdevel@...r.kernel.org,
        Greg Ungerer <gerg@...inux.org>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Giulio Benetti <giulio.benetti@...ettiengineering.com>
Subject: Re: [PATCH] proc: nommu: /proc/<pid>/maps: release mmap read lock

On Fri, Sep 15, 2023 at 02:15:15PM +0200, Oleg Nesterov wrote:
> Sorry for the offtopic question. I know NOTHING about nommu and when I tried to
> review this patch I was puzzled by
> 
> 	/* See m_next(). Zero at the start or after lseek. */
> 	if (addr == -1UL)
> 		return NULL;
> 
> at the start of m_start(). OK, lets look at
> 
> 	static void *m_next(struct seq_file *m, void *_p, loff_t *pos)
> 	{
> 		struct vm_area_struct *vma = _p;
> 
> 		*pos = vma->vm_end;
> 		return find_vma(vma->vm_mm, vma->vm_end);
> 	}
> 
> where does this -1UL come from? Does this mean that on nommu
> 
> 	last_vma->vm_end == -1UL
> 
> or what?
> 
> fs/proc/task_mmu.c has the same check at the start, but in this case
> the "See m_next()" comment actually helps.

Yes, this is another copying mistake from the MMU implementation. In
fact, it turns out that no-MMU /proc/<pid>/maps is completely broken
after 0c563f148043 ("proc: remove VMA rbtree use from nommu"). It just
returns an empty file.

This happens because find_vma() doesn't do what we want here. It "look[s]
up the first VMA in which addr resides, NULL if none", and the address
will be zero in in m_start(), which makes find_vma() return NULL (unless
presumably the zero address is actually part of the process's address
space).

I didn't run into this because I developed my patch against an older
kernel, and didn't test the latest version until today.

I'm preparing a second patch to fix this bug.

> 
> Just curious, thanks.
> 
> Oleg.
> 

Thanks, Ben