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 3D895D0EE19 for ; Fri, 11 Oct 2024 18:47:07 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id CBEE86B00B1; Fri, 11 Oct 2024 14:47:06 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id C46F06B00B2; Fri, 11 Oct 2024 14:47:06 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id AE8836B00B4; Fri, 11 Oct 2024 14:47:06 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0015.hostedemail.com [216.40.44.15]) by kanga.kvack.org (Postfix) with ESMTP id 87C246B00B1 for ; Fri, 11 Oct 2024 14:47:06 -0400 (EDT) Received: from smtpin28.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay03.hostedemail.com (Postfix) with ESMTP id 50546A12E3 for ; Fri, 11 Oct 2024 18:46:57 +0000 (UTC) X-FDA: 82662203610.28.2704AB3 Received: from mail-lf1-f52.google.com (mail-lf1-f52.google.com [209.85.167.52]) by imf10.hostedemail.com (Postfix) with ESMTP id D99CEC001C for ; Fri, 11 Oct 2024 18:47:02 +0000 (UTC) Authentication-Results: imf10.hostedemail.com; dkim=pass header.d=google.com header.s=20230601 header.b=U+vrQNih; spf=pass (imf10.hostedemail.com: domain of jannh@google.com designates 209.85.167.52 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=1728672354; h=from:from:sender:reply-to:subject:subject:date:date: message-id:message-id:to:to:cc:mime-version:mime-version: content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references:dkim-signature; bh=eX3HkjI6Zaa2TFFdkwiH3pZsKN0shfauRmPc72yF3JA=; b=RvlTOtMgXkUgeh67vva1QIOBV8XiPHDMtJcfB8tBUjkB7LTbYEMcjGGaPV9kOlvn8TdaKH cpTuFTS4W78c47B2iqialGrcrpAneICPINOdxAK3of+jax39nheHNCGH9KoQ0Vm4q16+MD KTL6Nh3y8Sf+R7kVu/jGIjnbqlXfxZc= ARC-Authentication-Results: i=1; imf10.hostedemail.com; dkim=pass header.d=google.com header.s=20230601 header.b=U+vrQNih; spf=pass (imf10.hostedemail.com: domain of jannh@google.com designates 209.85.167.52 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=1728672354; a=rsa-sha256; cv=none; b=Ls6vmI7xpuMJzzn+tFoq4E7V6GOidNguM7aCdmJBnJ+ICprk4T+b+zOrTFoCflI2QIhiFu +A9FowwJOqwdg8w7EK8kirIQRhb6NJgm9BWPx8taCOqMsKUzqQW0FWWrjtUtPoyy9VX8y1 KLqx4MwCHHF0YxqZekGxRKNFPcbZAhc= Received: by mail-lf1-f52.google.com with SMTP id 2adb3069b0e04-5398c2e4118so3709e87.0 for ; Fri, 11 Oct 2024 11:47:03 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20230601; t=1728672422; x=1729277222; darn=kvack.org; h=content-transfer-encoding:to:subject:message-id:date:from :in-reply-to:references:mime-version:from:to:cc:subject:date :message-id:reply-to; bh=eX3HkjI6Zaa2TFFdkwiH3pZsKN0shfauRmPc72yF3JA=; b=U+vrQNihYsuOUqAOgOo5eXCRr5+gDdqLJOhtME04gbEsoIEWaa6FOgjTnykwyP1mSo KMJMv3DTvMYF0Wonk/djDJ7Xp3MC4jl89vu4KT3nMuMq1VzR1maOYUKNlopzA7pbonqT 9+cogY+QrAs7zH0UTFTo7A2mMeqYBHai7y4MvFjMoWtjM4OFJ0XdzRYQNg+9a0EuKpZ6 lHKLLquG+1gijgVMNVuVGELKsCp1V78vAVdI1FgSF7BGNfYvAnW996roKSKoshqOcFg8 FlbvFJ2xnCMn1/pi6qt8QD2haRC4pjY43Fm0Lzu5uKf3Z3/FAgWL2R7n6e4J2gxvAase rsyg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1728672422; x=1729277222; h=content-transfer-encoding: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=eX3HkjI6Zaa2TFFdkwiH3pZsKN0shfauRmPc72yF3JA=; b=cSvaWzw7oDt6RQNqbRk+yQXyeb2dHLgV87uX107NuW1TvEWOYJplOg7DIbWvbDHQWW QXUKCWXhyGNLFvy9cadHBpz6Jm3oeRdxlc0oPwkDXW6Ajt1MGRWCn3CYTF2bpo1DJv5s 0s16lAg2YbsPyOozcry3Pb3GIWGdWMQ0yg27xA7FuUzOn0qhaWOvIh9yaP80XoD6qWiu wyVfIHn/clIWxxwzr25Kai7RGAvEKMkVVI4l0nop7g2Pz4KTRqg8gNkNiu9KNgUXZgQk YjKp/H0O3fdqfwoPndEqB2o6Gq4avYYfhvH9yKEOawvZDgpcx1AgBpg0gG1J+DKHhpl8 7Z9w== X-Forwarded-Encrypted: i=1; AJvYcCXKGngUgMbnU6zsjB3X56CJ1UPuOGb/MMP3Vtwx84tw8P8kFXmO7e15OqZlxNoaLLQy3ZVgXAun4w==@kvack.org X-Gm-Message-State: AOJu0YxyPUOjU4jpxmzGerkjmFrnja/Ftt9qTRi6N/jufNO2kRAMeirp YpMAhqotLraJlA3NxipYMD9jewD5lxxEMZm8J0wJ6dFyhAy61keq+GUY4GpNkuUjsZ8cwK6mYGW AJZx7flDLbpcFVJyxzM+eU0ekkOACX2WT2cwV X-Google-Smtp-Source: AGHT+IEpsGIBekk+TDxVRpGTKk+rvvy2DrAz7Z6lJU9X6FNw2REhZ2lq2Ony9IlWMCI/Pg8y632cDd0FBh1e/rpHWXo= X-Received: by 2002:a05:6512:acf:b0:52e:8475:7c23 with SMTP id 2adb3069b0e04-539e60a65ebmr35995e87.7.1728672421904; Fri, 11 Oct 2024 11:47:01 -0700 (PDT) MIME-Version: 1.0 References: <20241011-stack-gap-inaccessible-v2-1-111b6a0ee2cb@google.com> In-Reply-To: From: Jann Horn Date: Fri, 11 Oct 2024 20:46:24 +0200 Message-ID: Subject: Re: [PATCH RFC v2] mm: Enforce the stack gap when changing inaccessible VMAs To: "Liam R. Howlett" , Jann Horn , Andrew Morton , Lorenzo Stoakes , Vlastimil Babka , Hugh Dickins , Oleg Nesterov , Michal Hocko , Helge Deller , Ben Hutchings , Willy Tarreau , Rik van Riel , linux-mm@kvack.org, linux-kernel@vger.kernel.org, stable@vger.kernel.org Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Rspamd-Server: rspam03 X-Rspam-User: X-Rspamd-Queue-Id: D99CEC001C X-Stat-Signature: 648rffr7rfooirec8c1dskidybbeekqb X-HE-Tag: 1728672422-918802 X-HE-Meta: U2FsdGVkX18LNfKZsOW8RNfYLwUbZdLwldmQUi610kb/QRBF+Tgjq9ImNHncvxZVDcM0+ow4k2BJcUoJYwpf0F9mxIwVuEyJfWqvdwQkf2Mn9OrNgwiynHg7uX0b82GrnYWsbd6EvHdESHleq4dMpTttam2WujgR+bVYxXyH5toa0AQcgs3JY+Bttw5Bf9F0TWRY4RDiWRfxnhjdeHEuZr7b2hvqUKr1rM2Wi4CgTnglWnBjlh4FRBcrxkbeZ3lkke1MyX5F85j/qa+XcoKKv/Ej/DHHvAFdX+mcc1wmBK0yJFEvTeYsw0pTswKCCOOTlhpu9CjhYcZ9x39gRwM/ksRFKr+tTSDNwWujZc4k4v5Ho8wuc2NeTtq03tb8PLHfh+l0gNgA/fMOhQX7//d/pbIsD0eqBiR3hbYnl+I2gCpVAi05QxwRgmXWrmr5rWN7SOrPKJAfS5WQxc6/IAZyE7emx/6IISaOEWRKJQWBu/nrKfgGXs9Tp5wsmikfRAvT1c99/QrRn+uGCKgW0Eykiy74g/C48F7QH5C77noyw9m+I8G+Gc1YpkD8mov6BKkJvnEpMgEGXU8S8ExPZnm/0Gax8OA2ZaTegsm2NAYXZlFy+DXtTj73f0KpJuZT9mFQp4vkYiqJV7Oxm9+qk86V1Kh1cXgLQENq+x/vovl/zR3rLT4gc7qqiyspUMaUqmcYZoedQwMWsHYPMb4+afHq2sNOvzXlbbzg6PLKD/G8v4h7nNeIF7No0rBI+RQwRK3hG8P+rNcWp490HaWqPJipNLsDS8P+e/6bpSIRfA1wl3e/VRUNob9ErrRKDbztRzUKwcd5YVCqb4SyzekHLEe+ouC3eVPP11Kb9t22xHHkMwZL8l3GlcA4g8sc7TEhK4kNBDPAI2HheeyEJYbURHy1Kg+ufwEyS7/WLkpExoTMLUrCVhA/WWUbnZTfEcdvOrHtQF03VVyhPtcIUUo3M6c 0fbiTT55 9KFeE41lmJb0W2ycyU/ExxLb610PGOwvqszfLjPUhMRs+GuzzCpmPtsjawq3DhEbFWj7WaxTqYTCjfs0gh+nMCLxfBIh+8OPuqq8OEkbJ4Br6ujNQM0vO7qS5WATX5s+YaT82up7gPdwc4Wx/Sscyq8wo47JFflMq0hgKU4uo4ceEehzh4X0W2DPH83KgaYbpsR+eTFuew3+HLiNKc4K3lWnx98q7rugnmk0nv9m2YaokIp7xc4KhEPRVgw== 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: On Fri, Oct 11, 2024 at 7:55=E2=80=AFPM Liam R. Howlett wrote: > * Jann Horn [241011 11:51]: > > 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. > > > > We should ensure that, as long as code is compiled with something like > > -fstack-check, a stack overflow in this code can never cause the main s= tack > > to overflow into adjacent heap memory - so the bottom of a stack should > > never be directly adjacent to an accessible VMA. [...] > > diff --git a/mm/mmap.c b/mm/mmap.c > > index dd4b35a25aeb..937361be3c48 100644 > > --- a/mm/mmap.c > > +++ b/mm/mmap.c > > @@ -359,6 +359,20 @@ unsigned long do_mmap(struct file *file, unsigned = long addr, > > return -EEXIST; > > } > > > > + /* > > + * This does two things: > > + * > > + * 1. Disallow MAP_FIXED replacing a PROT_NONE VMA adjacent to a = stack > > + * with an accessible VMA. > > + * 2. Disallow MAP_FIXED_NOREPLACE creating a new accessible VMA > > + * adjacent to a stack. > > + */ > > + if ((flags & (MAP_FIXED_NOREPLACE | MAP_FIXED)) && > > + (prot & (PROT_READ | PROT_WRITE | PROT_EXEC)) && > > + !(vm_flags & (VM_GROWSUP|VM_GROWSDOWN)) && > > + overlaps_stack_gap(mm, addr, len)) > > + return (flags & MAP_FIXED) ? -ENOMEM : -EEXIST; > > + > > This is probably going to impact performance for allocators by causing > two walks of the tree any time they protect a portion of mmaped area. Well, it's one extra walk except on parisc, thanks to the "if (!IS_ENABLED(CONFIG_STACK_GROWSUP))" bailout - but point taken, it would be better to avoid that. > In the mmap_region() code, there is a place we know next/prev on > MAP_FIXED, and next for MAP_FIXED_NOREPLACE - which has a vma iterator > that would be lower cost than a tree walk. That area may be a better > place to check these requirements. Unfortunately, it may cause a vma > split in the vms_gather_munmap_vmas() call prior to this check, but > considering the rarity it may not be that big of a deal? Hmm, yeah, that sounds fine to me. [...] > > diff --git a/mm/mprotect.c b/mm/mprotect.c > > index 0c5d6d06107d..2300e2eff956 100644 > > --- a/mm/mprotect.c > > +++ b/mm/mprotect.c > > @@ -772,6 +772,12 @@ static int do_mprotect_pkey(unsigned long start, s= ize_t len, > > } > > } > > > > + error =3D -ENOMEM; > > + if ((prot & (PROT_READ | PROT_WRITE | PROT_EXEC)) && > > + !(vma->vm_flags & (VM_GROWSUP|VM_GROWSDOWN)) && > > + overlaps_stack_gap(current->mm, start, end - start)) > > + goto out; > > + > > We have prev just below your call here, so we could reuse that. Getting > the vma after the mprotect range doesn't seem that easy. I guess we > need to make the loop even more complicated and find the next vma (and > remember the fixup can merge). This isn't as straight forward as what > you have, but would be faster. For mprotect, maybe one option would be to do it inside the loop? Something like this: ``` diff --git a/mm/mprotect.c b/mm/mprotect.c index d0e3ebfadef8..2873cc254eaf 100644 --- a/mm/mprotect.c +++ b/mm/mprotect.c @@ -790,6 +790,24 @@ static int do_mprotect_pkey(unsigned long start, size_t len, break; } + if (IS_ENABLED(CONFIG_STACK_GROWSUP) && vma->vm_start =3D=3D start) { + /* just do an extra lookup here, we do this only on parisc */ + if (overlaps_stack_gap_growsup([...])) { + error =3D -ENOMEM; + break; + } + } + if (vma->vm_end =3D=3D end) { + /* peek ahead */ + struct vma_iterator vmi_peek =3D vmi; + struct vm_area_struct *next =3D vma_next(&vmi_peek)= ; + + if (next && overlaps_stack_gap_growsdown([...], nex= t)) { + error =3D -ENOMEM; + break; + } + } + /* Does the application expect PROT_READ to imply PROT_EXEC= */ if (rier && (vma->vm_flags & VM_MAYEXEC)) prot |=3D PROT_EXEC; ``` Assuming that well-behaved userspace only calls mprotect() ranges that are fully covered by VMAs, that should be good enough? (I don't know how you feel about the idea of peeking ahead from a VMA iterator by copying the iterator, I imagine you might have a better way to do that...) > > prev =3D vma_prev(&vmi); > > if (start > vma->vm_start) > > prev =3D vma;