linux-fsdevel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: "Magnus Groß" <magnus.gross@rwth-aachen.de>
To: Alexander Viro <viro@zeniv.linux.org.uk>
Cc: Eric Biederman <ebiederm@xmission.com>,
	Kees Cook <keescook@chromium.org>,
	<linux-fsdevel@vger.kernel.org>
Subject: [PATCH] elf: Relax assumptions about vaddr ordering
Date: Wed, 26 Jan 2022 17:25:20 +0100	[thread overview]
Message-ID: <YfF18Dy85mCntXrx@fractal.localdomain> (raw)

From ff4dde97e82727727bda711f2367c05663498b24 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Magnus=20Gro=C3=9F?= <magnus.gross@rwth-aachen.de>
Date: Wed, 26 Jan 2022 16:35:07 +0100
Subject: [PATCH] elf: Relax assumptions about vaddr ordering
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Commit 5f501d555653 ("binfmt_elf: reintroduce using
MAP_FIXED_NOREPLACE") introduced a regression, where the kernel now
assumes that PT_LOAD segments are ordered by vaddr in load_elf_binary().

Specifically consider an ELF binary with the following PT_LOAD segments:

Type  Offset   VirtAddr   PhysAddr   FileSiz  MemSiz    Flg Align
LOAD  0x000000 0x08000000 0x08000000 0x474585 0x474585  R E 0x1000
LOAD  0x475000 0x08475000 0x08475000 0x090a4  0xc6c10   RW  0x1000
LOAD  0x47f000 0x00010000 0x00010000 0x00000  0x7ff0000     0x1000

Note how the last segment is actually the first segment and vice versa.

Since total_mapping_size() only computes the difference between the
first and the last segment in the order that they appear, it will return
a size of 0 in this case, thus causing load_elf_binary() to fail, which
did not happen before that change.

Strictly speaking total_mapping_size() made that assumption already
before that patch, but the issue did not appear because the old
load_addr_set guards never allowed this call to total_mapping_size().

Instead of fixing this by reverting to the old load_addr_set logic, we
fix this by comparing the correct first and last segments in
total_mapping_size().

Signed-off-by: Magnus Groß <magnus.gross@rwth-aachen.de>
---
 fs/binfmt_elf.c | 18 ++++++++++++++----
 1 file changed, 14 insertions(+), 4 deletions(-)

diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c
index f8c7f26f1fbb..0caaad9eddd1 100644
--- a/fs/binfmt_elf.c
+++ b/fs/binfmt_elf.c
@@ -402,19 +402,29 @@ static unsigned long elf_map(struct file *filep, unsigned long addr,
 static unsigned long total_mapping_size(const struct elf_phdr *cmds, int nr)
 {
 	int i, first_idx = -1, last_idx = -1;
+	unsigned long min_vaddr = ULONG_MAX, max_vaddr = 0;
 
 	for (i = 0; i < nr; i++) {
 		if (cmds[i].p_type == PT_LOAD) {
-			last_idx = i;
-			if (first_idx == -1)
+			/*
+			 * The PT_LOAD segments are not necessarily ordered
+			 * by vaddr. Make sure that we get the segment with
+			 * minimum vaddr (maximum vaddr respectively)
+			 */
+			if (cmds[i].p_vaddr <= min_vaddr) {
 				first_idx = i;
+				min_vaddr = cmds[i].p_vaddr;
+			}
+			if (cmds[i].p_vaddr >= max_vaddr) {
+				last_idx = i;
+				max_vaddr = cmds[i].p_vaddr;
+			}
 		}
 	}
 	if (first_idx == -1)
 		return 0;
 
-	return cmds[last_idx].p_vaddr + cmds[last_idx].p_memsz -
-				ELF_PAGESTART(cmds[first_idx].p_vaddr);
+	return max_vaddr + cmds[last_idx].p_memsz - ELF_PAGESTART(min_vaddr);
 }
 
 static int elf_read(struct file *file, void *buf, size_t len, loff_t pos)
-- 
2.34.1

             reply	other threads:[~2022-01-26 16:26 UTC|newest]

Thread overview: 13+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-01-26 16:25 Magnus Groß [this message]
2022-01-26 16:50 ` [PATCH] elf: Relax assumptions about vaddr ordering Kees Cook
2022-01-27  6:31   ` Kees Cook
2022-01-28  8:26     ` Magnus Groß
2022-01-28 20:04       ` Kees Cook
2022-01-28 22:30       ` Kees Cook
2022-01-29  7:53         ` Alexey Dobriyan
2022-01-29 13:28         ` Magnus Groß
2022-02-01 22:48         ` Andrew Morton
2022-02-02  1:07           ` Kees Cook
2022-02-02 15:15           ` Alexey Dobriyan
2022-02-02 15:44             ` Magnus Groß
2022-02-01 22:44     ` Andrew Morton

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=YfF18Dy85mCntXrx@fractal.localdomain \
    --to=magnus.gross@rwth-aachen.de \
    --cc=ebiederm@xmission.com \
    --cc=keescook@chromium.org \
    --cc=linux-fsdevel@vger.kernel.org \
    --cc=viro@zeniv.linux.org.uk \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).