From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by smtp.lore.kernel.org (Postfix) with ESMTP id EE28BCEF17C for ; Tue, 8 Oct 2024 14:20:56 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 77C726B0083; Tue, 8 Oct 2024 10:20:56 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 72D1D6B0085; Tue, 8 Oct 2024 10:20:56 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 625B66B0089; Tue, 8 Oct 2024 10:20:56 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0016.hostedemail.com [216.40.44.16]) by kanga.kvack.org (Postfix) with ESMTP id 3E7936B0083 for ; Tue, 8 Oct 2024 10:20:56 -0400 (EDT) Received: from smtpin14.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay02.hostedemail.com (Postfix) with ESMTP id 7DB45120248 for ; Tue, 8 Oct 2024 14:20:54 +0000 (UTC) X-FDA: 82650646470.14.7D866FA Received: from mail-wm1-f45.google.com (mail-wm1-f45.google.com [209.85.128.45]) by imf03.hostedemail.com (Postfix) with ESMTP id A905A2001A for ; Tue, 8 Oct 2024 14:20:53 +0000 (UTC) Authentication-Results: imf03.hostedemail.com; dkim=pass header.d=google.com header.s=20230601 header.b=grXOtNX4; spf=pass (imf03.hostedemail.com: domain of jannh@google.com designates 209.85.128.45 as permitted sender) smtp.mailfrom=jannh@google.com; dmarc=pass (policy=reject) header.from=google.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1728397210; h=from:from:sender:reply-to:subject:subject:date:date: message-id:message-id:to:to:cc:cc:mime-version:mime-version: content-type:content-type:content-transfer-encoding: in-reply-to:in-reply-to:references:references:dkim-signature; bh=5L57COaQpnN95Wg6Zd7qNnoi5oHy8p/sLDRMT2+LNOQ=; b=3BQgemwdajGSkPa6FFC6yBq7ADYtHfokTF3kCHL3BNPBh7P+VLqfQAoKqKWFG20Ep70eDO YqgCGyb7zuviDe3mAn0kwYJ815iWwq7e72iz3YEom8vKKvQGM30BHYfyCeRmnG3qY/dMXW 4zc4E0v1gcuA/pLnlkthtlk5t1OlZwU= ARC-Authentication-Results: i=1; imf03.hostedemail.com; dkim=pass header.d=google.com header.s=20230601 header.b=grXOtNX4; spf=pass (imf03.hostedemail.com: domain of jannh@google.com designates 209.85.128.45 as permitted sender) smtp.mailfrom=jannh@google.com; dmarc=pass (policy=reject) header.from=google.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1728397210; a=rsa-sha256; cv=none; b=3PQZWPmkFMJKv9k/H9QI9Kg+4HK2nPTEQeoz0PDroF+pk95mJY8VH6vadKtW5FLOVr140U wPQZiLtRLehlK/OyBn4WQFwXU5T/J0DR0F+EG8pTIb5hvQeKn5f9lwLxPYW5wGrvKXVbe2 4XlBuL2PqkY/8Hk5tByDCvStqxq3VmM= Received: by mail-wm1-f45.google.com with SMTP id 5b1f17b1804b1-42f6995dab8so282305e9.0 for ; Tue, 08 Oct 2024 07:20:53 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20230601; t=1728397252; x=1729002052; darn=kvack.org; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:from:to:cc:subject:date:message-id:reply-to; bh=5L57COaQpnN95Wg6Zd7qNnoi5oHy8p/sLDRMT2+LNOQ=; b=grXOtNX4T0RLhtka60w4qhrmDQFkdAPw6kDBqxPl0KnxwusSHIeepy+8Go6XFgEilY 4jRl9U0Vzpsehcn/X757/gmLrGF8ywxULimDHvM2jUzzLLS+4XTBjM9FsOoBPJiXSREX QM/VoorkV5ZWbZhlY2ZUIcZCfUVJq9a0QikNe+TdXyTGQhvBxfyi0UsrnTfc8qAzfSXY 93dWkcPrpy6qCn/R4xp5FwUwv7wiWB/iwVHc4iG3qsyy9qLEDQnzn/YuqncPEq/ezbnG oNiLJ3z9v+MQh2HjZ2tbsz7zhP73t2cAC71j9g0TYF7ahNbAWANU3k87NeWY9D5bDl4o wNUg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1728397252; x=1729002052; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=5L57COaQpnN95Wg6Zd7qNnoi5oHy8p/sLDRMT2+LNOQ=; b=hChN61OORs1hW5DF1aY3N6mtjAk/wvYU0QHKnueuTEV/zQ3TdLyc77zL2tCN/+ZoTE aqgu/srkRe7+gNkvN/jz0OoWF79EmvGj1uDGGLv1jl674R+lXkXDvU2AB4WaFlwdJrfg puKuDwSFidBwIu/5Cs04p0ahNni0k3zfdqI2cGQyUhZ9ABO3/dSvviMaDyhtdF3JzDJQ WFExsUp/stTYuesbnpmwErj58g5EuntYA8hZ6bdmmghMGb3I6oAhuWiG0Unfh4C+Rrf5 e/1yE5y0er7QsaycSvIs33/bEEOs6SZgnbZSkYpyGv0WTT/ggpZe+W3hfHcGW+LQINS9 46cQ== X-Forwarded-Encrypted: i=1; AJvYcCUFRBCDNXOaDRDHB0m1E7w6Pa+xweB0yB3gL+sU+nvmD+SxXcXGatWAGdne12lUGhyRDidST2rFKg==@kvack.org X-Gm-Message-State: AOJu0YySjQMKPFEgWzHOR/Fd8ZsORMAt7gR2FCKY9eJ1E4HyMhlKNJEL tKygAXjQA1kk8R//qEa3LnIry2roeRTHh46T2QLqTvnh8tqxV82fz21TW+u1WrKVE2kOM6AJtfz NYJ27PITQot2IkXrJRAz59ogUkzaHfMujqJi+ X-Google-Smtp-Source: AGHT+IGQGpc+2NdoSgKEGygdRbRQb4CH7S7QnY7D6okjNMK1pfRAP/EMjV3MND1mqjwNOyf5wx+LAoxWUboQj00WPHA= X-Received: by 2002:a05:600c:1d85:b0:42b:8ff7:bee2 with SMTP id 5b1f17b1804b1-4303de8bd79mr4008595e9.5.1728397251606; Tue, 08 Oct 2024 07:20:51 -0700 (PDT) MIME-Version: 1.0 References: <20241008-stack-gap-inaccessible-v1-1-848d4d891f21@google.com> <1eda6e90-b8d1-4053-9757-8f59c3a6e7ee@lucifer.local> In-Reply-To: <1eda6e90-b8d1-4053-9757-8f59c3a6e7ee@lucifer.local> From: Jann Horn Date: Tue, 8 Oct 2024 16:20:13 +0200 Message-ID: Subject: Re: [PATCH] mm: Enforce a minimal stack gap even against inaccessible VMAs To: Lorenzo Stoakes Cc: Andrew Morton , Hugh Dickins , Oleg Nesterov , Michal Hocko , Helge Deller , Vlastimil Babka , Ben Hutchings , Willy Tarreau , Rik van Riel , linux-mm@kvack.org, linux-kernel@vger.kernel.org, stable@vger.kernel.org, Liam Howlett Content-Type: multipart/mixed; boundary="000000000000376d5d0623f7d84b" X-Rspam-User: X-Stat-Signature: r4ektzfsrsupmwogi3z9puiibfss4cws X-Rspamd-Queue-Id: A905A2001A X-Rspamd-Server: rspam11 X-HE-Tag: 1728397253-937340 X-HE-Meta: U2FsdGVkX1/IuNN3IxmVZykGJwFfPelLxfnVEBudCARgPktKewPCRAkgEj3Vp8qXY3WFmNjFgSN44kZl+00aeZU5MEz88l+eOgRJRsbCAEaUR7MHSv/VWcXVv6I3cTY+/WpWGw4f/NxslfSdVD/nntfOw1egBV1IMkyAsH7ZXr+JwWUHqz1DbMPUP9MmNY9t8Z7YgtkO23ld9pBTpVV7oQNP0ypISbbdk4hlYcJ08FedBGBpd9FIF6FeJ/U79s8H5ZrSmDymq3oddZYeAA0cJQGpkYtu/YzK8znU+2c6u33DTiDORSFHBoTR7Awt8wuTX/+oyuN7J2HgbKtSlqh7qOQQpaNY69x48Ok+t5UAuUI+zs/LMYgRKY4QYV0T+ncSgJPvJTaq4oC2mtMfL9+yvHGkaadLiGmldnqMjgry/bvdFYQgR2ukN8oL3WFPevKO2+HTrzB/tmqYvnwtvZdKaDhs1jr10Ca8y+VwFyB+ZtaBqzIddEoPIJ6Y02T0B9jNBFA/+EtFGtZaP7VxOWAk/ZyChZ9ozY1DoJTe90z0jl7alv//BMpyB0uDaOcWcCgU1swccbZoDv1KP5/RRameQEWokDbNVYEnaCET8q6ZvWAiv1Ti/KnrZlSpFdLguAYT1Q4JuVAJtk2BKpflT+F75exwIbSYZ8XFnAEjJTTtUABYZPuS0qPbX8HpWG92/DN7f7YLgSXLGiu/Srn7BlY+RQX5f3zUxPA0IGoKYuyWVQVE4VCHHRKtV3yTo7SDpfaPnQ3kAUTSpA1JyVgOSRPt9ZLa9Nn8rQK/AyMoAPNVKyYem3T44MZ9a8jGyJuKuES9yIy3HyRmepvKAhd/FLcg92ITiny3Z08HUugHYsJ+RdLBa5Y0UbKAQZINxLam0S6npfgvUY9ae/gg8hP5cbJrWhzXlkGExwv+DTGr6BfggenGkI5/cIKuHEGuIsp3GsIJPk5S4eAIaVYGA+hzZqk ZDirseQ7 6Beo4NuYEs8lCBEpvzCwelTyOjzEYuwbXKuBbPBJxrikI1tTKJx1sSgjXnGs8mSSOY0k5bo3OUiVFa6MicKd3sGnFI2VpTTqKNGqz8MtbOF6tgZxngunHKP5otmNBQRsy9qTxz1BBcfzL6REhDzkihfVOLihS4FOWaJbKCqTBPLsXu5cKZS39TySSnpdLP4UTZT12ClnCrW/tUUh9ZbKv6TanZyGYudDyrWd8o3oVzcUiWYaYMfzdua3VbjUXOwM4cyIMYIDx4iUhIdjACaUFXC1MXnp1Spr4M0/SDhhUsmcKxz0sorR2b7tlI8p+iDG5XYot4STDHwCsIvEzw3+YkPdMnFU49bRjBhoVq4hF3UMnmLqoAFSXooyKt3fEoIrzjjfqbcgpKVWF4vJnyKXguHOlNwbyJMCdY8mMqIRvRL2NlhVwpRLP2XHkt2i0qZiV4ILXQK4PPLCCUWdXKFhjW2w1x3tUnngrmKU2rW9bOLcTZB9ZCE0h5rnGP17/Wj7tbdIZGROxrB+YgsQq4cwcrlKebTBf5/leUDp1SkHszA05gqI= X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: List-Subscribe: List-Unsubscribe: --000000000000376d5d0623f7d84b Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On Tue, Oct 8, 2024 at 1:40=E2=80=AFPM Lorenzo Stoakes wrote: > This is touching mm/mmap.c, please ensure to cc- the reviewers (me and > Liam, I have cc'd Liam here) as listed in MAINTAINERS when submitting > patches for this file. Ah, my bad, sorry about that. > Also this seems like a really speculative 'please discuss' change so shou= ld > be an RFC imo. > > On Tue, Oct 08, 2024 at 12:55:39AM +0200, Jann Horn wrote: > > As explained in the comment block this change adds, we can't tell what > > userspace's intent is when the stack grows towards an inaccessible VMA. > > > > I have a (highly contrived) C testcase for 32-bit x86 userspace with gl= ibc > > that mixes malloc(), pthread creation, and recursion in just the right = way > > such that the main stack overflows into malloc() arena memory. > > (Let me know if you want me to share that.) > > You are claiming a fixes from 2017 and cc'ing stable on a non-RFC so > yeah... we're going to need more than your word for it :) we will want to > be really sure this is a thing before we backport to basically every > stable kernel. > > Surely this isn't that hard to demonstrate though? You mmap something > PROT_NONE just stack gap below the stack, then intentionally extend stack > to it, before mprotect()'ing the PROT_NONE region? I've attached my test case that demonstrates this using basically only malloc, free, pthread_create() and recursion, plus a bunch of ugly read-only gunk and synchronization. It assumes that it runs under glibc, as a 32-bit x86 binary. Usage: $ clang -O2 -fstack-check -m32 -o grow-32 grow-32.c -pthread -ggdb && for i in {0..10}; do ./grow-32; done corrupted thread_obj2 at depth 190528 corrupted thread_obj2 at depth 159517 corrupted thread_obj2 at depth 209777 corrupted thread_obj2 at depth 200119 corrupted thread_obj2 at depth 208093 corrupted thread_obj2 at depth 167705 corrupted thread_obj2 at depth 234523 corrupted thread_obj2 at depth 174528 corrupted thread_obj2 at depth 223823 corrupted thread_obj2 at depth 199816 grow-32: malloc failed: Cannot allocate memory This demonstrates that it is possible for a userspace program that is just using basic libc functionality, and whose only bug is unbounded recursion, to corrupt memory despite being built with -fstack-check. As you said, to just demonstrate the core issue in a more contrived way, you can also use a simpler example: $ cat basic-grow-repro.c #include #include #include #define STACK_POINTER() ({ void *__stack; asm volatile("mov %%rsp, %0":"=3Dr"(__stack)); __stack; }) int main(void) { char *ptr =3D (char*)( (unsigned long)(STACK_POINTER() - (1024*1024*4)/*4MiB*/) & ~0xfffUL ); if (mmap(ptr, 0x1000, PROT_NONE, MAP_ANONYMOUS|MAP_PRIVATE, -1, 0) !=3D p= tr) err(1, "mmap"); *(volatile char *)(ptr + 0x1000); /* expand stack */ if (mprotect(ptr, 0x1000, PROT_READ|PROT_WRITE|PROT_EXEC)) err(1, "mprotect"); system("cat /proc/$PPID/maps"); } $ gcc -o basic-grow-repro basic-grow-repro.c $ ./basic-grow-repro 5600a0fef000-5600a0ff0000 r--p 00000000 fd:01 28313737 [...]/basic-grow-repro 5600a0ff0000-5600a0ff1000 r-xp 00001000 fd:01 28313737 [...]/basic-grow-repro 5600a0ff1000-5600a0ff2000 r--p 00002000 fd:01 28313737 [...]/basic-grow-repro 5600a0ff2000-5600a0ff3000 r--p 00002000 fd:01 28313737 [...]/basic-grow-repro 5600a0ff3000-5600a0ff4000 rw-p 00003000 fd:01 28313737 [...]/basic-grow-repro 7f9a88553000-7f9a88556000 rw-p 00000000 00:00 0 7f9a88556000-7f9a8857c000 r--p 00000000 fd:01 3417714 /usr/lib/x86_64-linux-gnu/libc.so.6 7f9a8857c000-7f9a886d2000 r-xp 00026000 fd:01 3417714 /usr/lib/x86_64-linux-gnu/libc.so.6 7f9a886d2000-7f9a88727000 r--p 0017c000 fd:01 3417714 /usr/lib/x86_64-linux-gnu/libc.so.6 7f9a88727000-7f9a8872b000 r--p 001d0000 fd:01 3417714 /usr/lib/x86_64-linux-gnu/libc.so.6 7f9a8872b000-7f9a8872d000 rw-p 001d4000 fd:01 3417714 /usr/lib/x86_64-linux-gnu/libc.so.6 7f9a8872d000-7f9a8873a000 rw-p 00000000 00:00 0 7f9a88754000-7f9a88756000 rw-p 00000000 00:00 0 7f9a88756000-7f9a8875a000 r--p 00000000 00:00 0 [v= var] 7f9a8875a000-7f9a8875c000 r-xp 00000000 00:00 0 [v= dso] 7f9a8875c000-7f9a8875d000 r--p 00000000 fd:01 3409055 /usr/lib/x86_64-linux-gnu/ld-linux-x86-64.so.2 7f9a8875d000-7f9a88782000 r-xp 00001000 fd:01 3409055 /usr/lib/x86_64-linux-gnu/ld-linux-x86-64.so.2 7f9a88782000-7f9a8878c000 r--p 00026000 fd:01 3409055 /usr/lib/x86_64-linux-gnu/ld-linux-x86-64.so.2 7f9a8878c000-7f9a8878e000 r--p 00030000 fd:01 3409055 /usr/lib/x86_64-linux-gnu/ld-linux-x86-64.so.2 7f9a8878e000-7f9a88790000 rw-p 00032000 fd:01 3409055 /usr/lib/x86_64-linux-gnu/ld-linux-x86-64.so.2 7fff84664000-7fff84665000 rwxp 00000000 00:00 0 7fff84665000-7fff84a67000 rw-p 00000000 00:00 0 [s= tack] $ Though, while writing the above reproducer, I noticed another dodgy scenario regarding the stack gap: MAP_FIXED_NOREPLACE apparently ignores the stack guard region, because it only checks for VMA intersection, see this example: $ cat basic-grow-repro-ohno.c #include #include #include #include #define STACK_POINTER() ({ void *__stack; asm volatile("mov %%rsp, %0":"=3Dr"(__stack)); __stack; }) int main(void) { setbuf(stdout, NULL); char *ptr =3D (char*)( (unsigned long)(STACK_POINTER() - (1024*1024*4)/*4MiB*/) & ~0xfffUL ); *(volatile char *)(ptr + 0x1000); /* expand stack to just above ptr */ printf("trying to map at %p\n", ptr); system("cat /proc/$PPID/maps;echo"); if (mmap(ptr, 0x1000, PROT_READ|PROT_WRITE|PROT_EXEC, MAP_FIXED_NOREPLACE|MAP_ANONYMOUS|MAP_PRIVATE, -1, 0) !=3D ptr) err(1, "mmap"); system("cat /proc/$PPID/maps"); } $ gcc -o basic-grow-repro-ohno basic-grow-repro-ohno.c $ ./basic-grow-repro-ohno trying to map at 0x7ffc344ca000 560ee371d000-560ee371e000 r--p 00000000 fd:01 28313842 [...]/basic-grow-repro-ohno 560ee371e000-560ee371f000 r-xp 00001000 fd:01 28313842 [...]/basic-grow-repro-ohno 560ee371f000-560ee3720000 r--p 00002000 fd:01 28313842 [...]/basic-grow-repro-ohno 560ee3720000-560ee3721000 r--p 00002000 fd:01 28313842 [...]/basic-grow-repro-ohno 560ee3721000-560ee3722000 rw-p 00003000 fd:01 28313842 [...]/basic-grow-repro-ohno 7f0d636ed000-7f0d636f0000 rw-p 00000000 00:00 0 7f0d636f0000-7f0d63716000 r--p 00000000 fd:01 3417714 /usr/lib/x86_64-linux-gnu/libc.so.6 7f0d63716000-7f0d6386c000 r-xp 00026000 fd:01 3417714 /usr/lib/x86_64-linux-gnu/libc.so.6 7f0d6386c000-7f0d638c1000 r--p 0017c000 fd:01 3417714 /usr/lib/x86_64-linux-gnu/libc.so.6 7f0d638c1000-7f0d638c5000 r--p 001d0000 fd:01 3417714 /usr/lib/x86_64-linux-gnu/libc.so.6 7f0d638c5000-7f0d638c7000 rw-p 001d4000 fd:01 3417714 /usr/lib/x86_64-linux-gnu/libc.so.6 7f0d638c7000-7f0d638d4000 rw-p 00000000 00:00 0 7f0d638ee000-7f0d638f0000 rw-p 00000000 00:00 0 7f0d638f0000-7f0d638f4000 r--p 00000000 00:00 0 [v= var] 7f0d638f4000-7f0d638f6000 r-xp 00000000 00:00 0 [v= dso] 7f0d638f6000-7f0d638f7000 r--p 00000000 fd:01 3409055 /usr/lib/x86_64-linux-gnu/ld-linux-x86-64.so.2 7f0d638f7000-7f0d6391c000 r-xp 00001000 fd:01 3409055 /usr/lib/x86_64-linux-gnu/ld-linux-x86-64.so.2 7f0d6391c000-7f0d63926000 r--p 00026000 fd:01 3409055 /usr/lib/x86_64-linux-gnu/ld-linux-x86-64.so.2 7f0d63926000-7f0d63928000 r--p 00030000 fd:01 3409055 /usr/lib/x86_64-linux-gnu/ld-linux-x86-64.so.2 7f0d63928000-7f0d6392a000 rw-p 00032000 fd:01 3409055 /usr/lib/x86_64-linux-gnu/ld-linux-x86-64.so.2 7ffc344cb000-7ffc348cd000 rw-p 00000000 00:00 0 [s= tack] 560ee371d000-560ee371e000 r--p 00000000 fd:01 28313842 [...]/basic-grow-repro-ohno 560ee371e000-560ee371f000 r-xp 00001000 fd:01 28313842 [...]/basic-grow-repro-ohno 560ee371f000-560ee3720000 r--p 00002000 fd:01 28313842 [...]/basic-grow-repro-ohno 560ee3720000-560ee3721000 r--p 00002000 fd:01 28313842 [...]/basic-grow-repro-ohno 560ee3721000-560ee3722000 rw-p 00003000 fd:01 28313842 [...]/basic-grow-repro-ohno 7f0d636ed000-7f0d636f0000 rw-p 00000000 00:00 0 7f0d636f0000-7f0d63716000 r--p 00000000 fd:01 3417714 /usr/lib/x86_64-linux-gnu/libc.so.6 7f0d63716000-7f0d6386c000 r-xp 00026000 fd:01 3417714 /usr/lib/x86_64-linux-gnu/libc.so.6 7f0d6386c000-7f0d638c1000 r--p 0017c000 fd:01 3417714 /usr/lib/x86_64-linux-gnu/libc.so.6 7f0d638c1000-7f0d638c5000 r--p 001d0000 fd:01 3417714 /usr/lib/x86_64-linux-gnu/libc.so.6 7f0d638c5000-7f0d638c7000 rw-p 001d4000 fd:01 3417714 /usr/lib/x86_64-linux-gnu/libc.so.6 7f0d638c7000-7f0d638d4000 rw-p 00000000 00:00 0 7f0d638ee000-7f0d638f0000 rw-p 00000000 00:00 0 7f0d638f0000-7f0d638f4000 r--p 00000000 00:00 0 [v= var] 7f0d638f4000-7f0d638f6000 r-xp 00000000 00:00 0 [v= dso] 7f0d638f6000-7f0d638f7000 r--p 00000000 fd:01 3409055 /usr/lib/x86_64-linux-gnu/ld-linux-x86-64.so.2 7f0d638f7000-7f0d6391c000 r-xp 00001000 fd:01 3409055 /usr/lib/x86_64-linux-gnu/ld-linux-x86-64.so.2 7f0d6391c000-7f0d63926000 r--p 00026000 fd:01 3409055 /usr/lib/x86_64-linux-gnu/ld-linux-x86-64.so.2 7f0d63926000-7f0d63928000 r--p 00030000 fd:01 3409055 /usr/lib/x86_64-linux-gnu/ld-linux-x86-64.so.2 7f0d63928000-7f0d6392a000 rw-p 00032000 fd:01 3409055 /usr/lib/x86_64-linux-gnu/ld-linux-x86-64.so.2 7ffc344ca000-7ffc344cb000 rwxp 00000000 00:00 0 7ffc344cb000-7ffc348cd000 rw-p 00000000 00:00 0 [s= tack] $ That could also be bad: MAP_FIXED_NOREPLACE exists, from what I understand, partly so that malloc implementations can use it to grow heap memory chunks (though glibc doesn't use it, I'm not sure who actually uses it that way). We wouldn't want a malloc implementation to grow a heap memory chunk until it is directly adjacent to a stack. > > I don't know of any specific scenario where this is actually exploitabl= e, > > but it seems like it could be a security problem for sufficiently unluc= ky > > userspace. > > > > I believe we should ensure that, as long as code is compiled with somet= hing > > like -fstack-check, a stack overflow in it can never cause the main sta= ck > > to overflow into adjacent heap memory. > > > > My fix effectively reverts the behavior for !vma_is_accessible() VMAs t= o > > the behavior before commit 1be7107fbe18 ("mm: larger stack guard gap, > > between vmas"), so I think it should be a fairly safe change even in > > case A. > > > > Fixes: 561b5e0709e4 ("mm/mmap.c: do not blow on PROT_NONE MAP_FIXED hol= es in the stack") > > Cc: stable@vger.kernel.org > > Signed-off-by: Jann Horn > > --- > > I have tested that Libreoffice still launches after this change, though > > I don't know if that means anything. > > > > Note that I haven't tested the growsup code. > > --- > > mm/mmap.c | 53 ++++++++++++++++++++++++++++++++++++++++++++++------- > > 1 file changed, 46 insertions(+), 7 deletions(-) > > > > diff --git a/mm/mmap.c b/mm/mmap.c > > index dd4b35a25aeb..971bfd6c1cea 100644 > > --- a/mm/mmap.c > > +++ b/mm/mmap.c > > @@ -1064,10 +1064,12 @@ static int expand_upwards(struct vm_area_struct= *vma, unsigned long address) > > gap_addr =3D TASK_SIZE; > > > > next =3D find_vma_intersection(mm, vma->vm_end, gap_addr); > > - if (next && vma_is_accessible(next)) { > > - if (!(next->vm_flags & VM_GROWSUP)) > > + if (next && !(next->vm_flags & VM_GROWSUP)) { > > + /* see comments in expand_downwards() */ > > + if (vma_is_accessible(prev)) > > + return -ENOMEM; > > + if (address =3D=3D next->vm_start) > > return -ENOMEM; > > - /* Check that both stack segments have the same anon_vma?= */ > > I hate that we even maintain this for one single arch I believe at this p= oint? Looks that way, just parisc? It would be so nice if we could somehow just get rid of this concept of growing stacks in general... > > } > > > > if (next) > > @@ -1155,10 +1157,47 @@ int expand_downwards(struct vm_area_struct *vma= , unsigned long address) > > /* Enforce stack_guard_gap */ > > prev =3D vma_prev(&vmi); > > /* Check that both stack segments have the same anon_vma? */ > > - if (prev) { > > - if (!(prev->vm_flags & VM_GROWSDOWN) && > > - vma_is_accessible(prev) && > > - (address - prev->vm_end < stack_guard_gap)) > > + if (prev && !(prev->vm_flags & VM_GROWSDOWN) && > > + (address - prev->vm_end < stack_guard_gap)) { > > + /* > > + * If the previous VMA is accessible, this is the normal = case > > + * where the main stack is growing down towards some unre= lated > > + * VMA. Enforce the full stack guard gap. > > + */ > > + if (vma_is_accessible(prev)) > > + return -ENOMEM; > > + > > + /* > > + * If the previous VMA is not accessible, we have a probl= em: > > + * We can't tell what userspace's intent is. > > + * > > + * Case A: > > + * Maybe userspace wants to use the previous VMA as a > > + * "guard region" at the bottom of the main stack, in whi= ch case > > + * userspace wants us to grow the stack until it is adjac= ent to > > + * the guard region. Apparently some Java runtime environ= ments > > + * and Rust do that? > > + * That is kind of ugly, and in that case userspace reall= y ought > > + * to ensure that the stack is fully expanded immediately= , but > > + * we have to handle this case. > > Yeah we can't break userspace on this, no doubt somebody is relying on th= is > _somewhere_. It would have to be a new user who appeared after commit 1be7107fbe18. And they'd have to install a "guard vma" somewhere below the main stack, and they'd have to care so much about the size of the stack that a single page makes a difference. > That said, I wish we disallowed this altogether regardless of accessibili= ty. > > > + * > > + * Case B: > > + * But maybe the previous VMA is entirely unrelated to th= e stack > > + * and is only *temporarily* PROT_NONE. For example, glib= c > > + * malloc arenas create a big PROT_NONE region and then > > + * progressively mark parts of it as writable. > > + * In that case, we must not let the stack become adjacen= t to > > + * the previous VMA. Otherwise, after the region later be= comes > > + * writable, a stack overflow will cause the stack to gro= w into > > + * the previous VMA, and we won't have any stack gap to p= rotect > > + * against this. > > Should be careful with terminology here, an mprotect() will not allow a > merge so by 'grow into' you mean that a stack VMA could become immediatel= y > adjacent to a non-stack VMA prior to it which was later made writable. > > Perhaps I am being pedantic... Ah, sorry, I worded that very confusingly. By "a stack overflow will cause the stack to grow into the previous VMA", I meant that the stack pointer moves into the previous VMA and the program uses part of the previous VMA as stack memory, clobbering whatever was stored there before. That part was not meant to talk about a change of VMA bounds. > > + * > > + * As an ugly tradeoff, enforce a single-page gap. > > + * A single page will hopefully be small enough to not be > > + * noticed in case A, while providing the same level of > > + * protection in case B that normal userspace threads get= . > > + */ > > + if (address =3D=3D prev->vm_end) > > return -ENOMEM; > > Ugh, yuck. Not a fan of this at all. Feels like a dreadful hack. Oh, I agree, I just didn't see a better way to do it. > You do raise an important point here, but it strikes me that we should be > doing this check in the mprotect()/mmap() MAP_FIXED scenarios where it > shouldn't be too costly to check against the next VMA (which we will be > obtaining anyway for merge checks)? > > That way we don't need a hack like this, and can just disallow the > operation. That'd probably be as liable to break the program as an -ENOME= M > on a stack expansion would... Hmm... yeah, I guess that would work. If someone hits this case, it would probably be less obvious to the programmer what went wrong based on the error code, but on the other hand, it would give userspace a slightly better chance to recover from the issue... I guess I can see if I can code that up. --000000000000376d5d0623f7d84b Content-Type: text/x-csrc; charset="US-ASCII"; name="grow-32.c" Content-Disposition: attachment; filename="grow-32.c" Content-Transfer-Encoding: base64 Content-ID: X-Attachment-Id: f_m20f55550 I2RlZmluZSBfR05VX1NPVVJDRQojaW5jbHVkZSA8cHRocmVhZC5oPgojaW5jbHVkZSA8c3RkaW8u aD4KI2luY2x1ZGUgPHN0ZGxpYi5oPgojaW5jbHVkZSA8dW5pc3RkLmg+CiNpbmNsdWRlIDxlcnIu aD4KI2luY2x1ZGUgPHN5cy9ldmVudGZkLmg+CgojZGVmaW5lIFNUQUNLX1BPSU5URVIoKSAoeyB2 b2lkICpfX3N0YWNrOyBhc20gdm9sYXRpbGUoIm1vdiAlJWVzcCwgJTAiOiI9ciIoX19zdGFjaykp OyBfX3N0YWNrOyB9KQojZGVmaW5lIElHTk9SRSh4KSBhc20gdm9sYXRpbGUoIiI6OiJyIih4KSkK CiNkZWZpbmUgTUFQX0xFTiAweDEwMDAwMAojZGVmaW5lIFVOTUFQX0NPVU5UIDEwCgpzdGF0aWMg aW50IGVmZF90b19tYWluLCBlZmRfdG9fdGhyZWFkOwpzdGF0aWMgdm9pZCAqbWFsbG9jX3N0YXJ0 LCAqbWFsbG9jX2VuZDsKc3RhdGljIHVuc2lnbmVkIGxvbmcgKnRocmVhZF9vYmoyOwoKc3RhdGlj IHZvaWQgKnRocmVhZF9mbih2b2lkICpkdW1teSkgewogIC8qIFNURVAgMiAqLwogIHZvaWQgKnRo cmVhZF9vYmogPSBtYWxsb2MoMHgxMDAwKTsKICB0aHJlYWRfb2JqMiA9IG1hbGxvYygweDEwMDAp OwogICp0aHJlYWRfb2JqMiA9IDB4ZGVhZGJlZWY7CiAgbWFsbG9jX3N0YXJ0ID0gKHZvaWQqKSgo KHVuc2lnbmVkIGxvbmcpdGhyZWFkX29iaikgJiB+MHhmZmZVTCk7CiAgbWFsbG9jX2VuZCA9IG1h bGxvY19zdGFydCArIE1BUF9MRU47CiAgZXZlbnRmZF93cml0ZShlZmRfdG9fbWFpbiwgMSk7CiAg ZXZlbnRmZF90IGR1bW15X3ZhbDsKICBldmVudGZkX3JlYWQoZWZkX3RvX3RocmVhZCwgJmR1bW15 X3ZhbCk7CgogIC8qIFNURVAgNCAqLwogIHdoaWxlICgxKSB7CiAgICB2b2lkICpwID0gbWFsbG9j KDB4NDAwKTsKICAgIGlmIChwIDwgbWFsbG9jX3N0YXJ0IHx8IHAgPiBtYWxsb2NfZW5kKQogICAg ICBicmVhazsKICB9CiAgZXZlbnRmZF93cml0ZShlZmRfdG9fbWFpbiwgMSk7CgogIHdoaWxlICgx KQogICAgcGF1c2UoKTsKfQoKX19hdHRyaWJ1dGVfXygobm9pbmxpbmUpKQp2b2lkIHJlY3Vyc2Uo KSB7CiAgaWYgKFNUQUNLX1BPSU5URVIoKSA+IG1hbGxvY19lbmQrMHhmMDApCiAgICByZWN1cnNl KCk7CiAgYXNtIHZvbGF0aWxlKCIiKTsKfQoKX19hdHRyaWJ1dGVfXygobm9pbmxpbmUpKQp2b2lk IHJlY3Vyc2UyKHVuc2lnbmVkIGxvbmcgZGVwdGgpIHsKICBpZiAoKnRocmVhZF9vYmoyICE9IDB4 ZGVhZGJlZWYpIHsKICAgIHByaW50ZigiY29ycnVwdGVkIHRocmVhZF9vYmoyIGF0IGRlcHRoICVs dVxuIiwgZGVwdGgpOwogICAgZXhpdCgxKTsKICB9CiAgcmVjdXJzZTIoZGVwdGgrMSk7CiAgYXNt IHZvbGF0aWxlKCIiKTsKfQoKdm9pZCAqdG9wX21hcHNbVU5NQVBfQ09VTlRdOwp2b2lkIHRvcF9t YXBzX2luc2VydCh2b2lkICpwdHIpIHsKICAvLyBmaW5kIGxvd2VzdCBlbGVtZW50IGluIHRvcF9t YXBzCiAgc2l6ZV90IGxvd2VzdCA9IDA7CiAgZm9yIChzaXplX3QgaSA9IDA7IGkgPCBVTk1BUF9D T1VOVDsgaSsrKSB7CiAgICBpZiAodG9wX21hcHNbaV0gPCB0b3BfbWFwc1tsb3dlc3RdKQogICAg ICBsb3dlc3QgPSBpOwogIH0KICBpZiAodG9wX21hcHNbbG93ZXN0XSA8IHB0cikKICAgIHRvcF9t YXBzW2xvd2VzdF0gPSBwdHI7Cn0KCmludCBtYWluKHZvaWQpIHsKICBlZmRfdG9fbWFpbiA9IGV2 ZW50ZmQoMCwgRUZEX1NFTUFQSE9SRSk7CiAgZWZkX3RvX3RocmVhZCA9IGV2ZW50ZmQoMCwgRUZE X1NFTUFQSE9SRSk7CgogIC8qIFNURVAgMSAqLwogIHZvaWQgKnN0YWNrID0gU1RBQ0tfUE9JTlRF UigpOwogIElHTk9SRShtYWxsb2MoMSkpOyAvKiBtYWtlIHN1cmUgb3VyIGFyZW5hIGlzIGluaXRp YWxpemVkIG9yIHdoYXRldmVyICovCiAgd2hpbGUgKDEpIHsKICAgIHZvaWQgKm9iaiA9IG1hbGxv YyhNQVBfTEVOIC0gMHhmMDApOwogICAgaWYgKG9iaiA9PSBOVUxMKQogICAgICBlcnIoMSwgIm1h bGxvYyBmYWlsZWQiKTsKICAgIGlmIChvYmogPiBTVEFDS19QT0lOVEVSKCkpIHsKICAgICAgZnJl ZShvYmopOwogICAgICBmb3IgKHNpemVfdCBpID0gMDsgaSA8IFVOTUFQX0NPVU5UOyBpKyspCiAg ICAgICAgZnJlZSh0b3BfbWFwc1tpXSk7CiAgICAgIGJyZWFrOwogICAgfQogICAgdG9wX21hcHNf aW5zZXJ0KG9iaik7CiAgfQogIHB0aHJlYWRfdCB0aHJlYWQ7CiAgaWYgKHB0aHJlYWRfY3JlYXRl KCZ0aHJlYWQsIE5VTEwsIHRocmVhZF9mbiwgTlVMTCkpCiAgICBlcnJ4KDEsICJwdGhyZWFkX2Ny ZWF0ZSBmYWlsZWQiKTsKICBldmVudGZkX3QgZHVtbXlfdmFsOwogIGV2ZW50ZmRfcmVhZChlZmRf dG9fbWFpbiwgJmR1bW15X3ZhbCk7CgogIC8qIFNURVAgMyAqLwogIHJlY3Vyc2UoKTsKICBldmVu dGZkX3dyaXRlKGVmZF90b190aHJlYWQsIDEpOwogIGV2ZW50ZmRfcmVhZChlZmRfdG9fbWFpbiwg JmR1bW15X3ZhbCk7CgogIC8qIFNURVAgNSAqLwogIHJlY3Vyc2UyKDApOwp9Cg== --000000000000376d5d0623f7d84b--