Re: [PATCH] procfs: fix mmap() for /proc/vmcore

[Linus Torvalds]

On Sat, May 19, 2018 at 4:39 AM Vasily Gorbik <gor@xxxxxxxxxxxxx> wrote:

> Would work, but file_mmap_size_max first checks
>              if (S_ISREG(inode->i_mode))
>                      return inode->i_sb->s_maxbytes;
> before
>              if (file->f_mode & FMODE_UNSIGNED_OFFSET)
>                      return 0;
> so, as it is this patch does not fix the issue.

Right you are.

We could easily reverse the order of those two checks, but since clearly
the s_maxbytes case hadn't gotten as much coverage as I thought it had,
let's just simplify file_mmap_size_max() instead, and just make the limit
be MAX_LFS_FILESIZE for regular files too, the way we already do for block
devices (instead of trying to figure out the size of the block device).

That way we won't be introducing changes to s_maxbytes late in the rc
series, and we'll only affect the new logic.

Plus regular files was never what that logic really was introduced to
protect against anyway. It's the random ad-hoc mmap implementations in
drivers that it really aimed to protect.

So for now, I've committed the minimal fixlet that doesn't affect any
existing code (just the new max file size logic), and maybe this can be
revisited for 4.18.

                Linus
From 423913ad4ae5b3e8fb8983f70969fb522261ba26 Mon Sep 17 00:00:00 2001
From: Linus Torvalds <torvalds@linux-foundation.org>
Date: Sat, 19 May 2018 09:29:11 -0700
Subject: [PATCH] mmap: relax file size limit for regular files

Commit be83bbf80682 ("mmap: introduce sane default mmap limits") was
introduced to catch problems in various ad-hoc character device drivers
doing mmap and getting the size limits wrong.  In the process, it used
"known good" limits for the normal cases of mapping regular files and
block device drivers.

It turns out that the "s_maxbytes" limit was less "known good" than I
thought.  In particular, /proc doesn't set it, but exposes one regular
file to mmap: /proc/vmcore.  As a result, that file got limited to the
default MAX_INT s_maxbytes value.

This went unnoticed for a while, because apparently the only thing that
needs it is the s390 kernel zfcpdump, but there might be other tools
that use this too.

Vasily suggested just changing s_maxbytes for all of /proc, which isn't
wrong, but makes me nervous at this stage.  So instead, just make the
new mmap limit always be MAX_LFS_FILESIZE for regular files, which won't
affect anything else.  It wasn't the regular file case I was worried
about.

I'd really prefer for maxsize to have been per-inode, but that is not
how things are today.

Fixes: be83bbf80682 ("mmap: introduce sane default mmap limits")
Reported-by: Vasily Gorbik <gor@linux.ibm.com>
Cc: Al Viro <viro@zeniv.linux.org.uk>
Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
---
 mm/mmap.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/mm/mmap.c b/mm/mmap.c
index 78e14facdb6e..fc41c0543d7f 100644
--- a/mm/mmap.c
+++ b/mm/mmap.c
@@ -1327,7 +1327,7 @@ static inline int mlock_future_check(struct mm_struct *mm,
 static inline u64 file_mmap_size_max(struct file *file, struct inode *inode)
 {
 	if (S_ISREG(inode->i_mode))
-		return inode->i_sb->s_maxbytes;
+		return MAX_LFS_FILESIZE;
 
 	if (S_ISBLK(inode->i_mode))
 		return MAX_LFS_FILESIZE;
-- 
2.17.0.392.g44117ef7f