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 60190D3DEAB for ; Fri, 18 Oct 2024 19:05:06 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id C23346B00B1; Fri, 18 Oct 2024 15:05:05 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id BD34E6B00B5; Fri, 18 Oct 2024 15:05:05 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id A9AF26B00B6; Fri, 18 Oct 2024 15:05:05 -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 8B1CA6B00B1 for ; Fri, 18 Oct 2024 15:05:05 -0400 (EDT) Received: from smtpin26.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay02.hostedemail.com (Postfix) with ESMTP id AFF37121C08 for ; Fri, 18 Oct 2024 19:04:53 +0000 (UTC) X-FDA: 82687650192.26.7D80519 Received: from dfw.source.kernel.org (dfw.source.kernel.org [139.178.84.217]) by imf29.hostedemail.com (Postfix) with ESMTP id EA327120022 for ; Fri, 18 Oct 2024 19:04:47 +0000 (UTC) Authentication-Results: imf29.hostedemail.com; dkim=pass header.d=kernel.org header.s=k20201202 header.b=Q5uvTqpE; dmarc=pass (policy=quarantine) header.from=kernel.org; spf=pass (imf29.hostedemail.com: domain of jarkko@kernel.org designates 139.178.84.217 as permitted sender) smtp.mailfrom=jarkko@kernel.org ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1729278268; a=rsa-sha256; cv=none; b=D+XUuKA/sqtmVpWPa4+1CZzC1L2J9MMrLfFcTS46fziDwK1BEqCF5TQ7KHw0BIjO2NvJxQ p5MHnaOO0pbWjuify2FiPHbW9Dq4Ezo50Ob+LVxMUBtI3pOStSFvZ2Rjl9vZd+FliOTSHx 4aHLpoG3qtnFP66dUJoGtMSXkLdgDXY= ARC-Authentication-Results: i=1; imf29.hostedemail.com; dkim=pass header.d=kernel.org header.s=k20201202 header.b=Q5uvTqpE; dmarc=pass (policy=quarantine) header.from=kernel.org; spf=pass (imf29.hostedemail.com: domain of jarkko@kernel.org designates 139.178.84.217 as permitted sender) smtp.mailfrom=jarkko@kernel.org ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1729278268; 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:content-transfer-encoding: in-reply-to:in-reply-to:references:references:dkim-signature; bh=8R2fBb4X85mjOZzvnCSXf0vV4Ejn09ptUUj2GGIs0pc=; b=W6JY4qNwR32a4qXxceEt6Ul8zPcrif3YIitGEoZMIEylSV2TbhhnRWGXDaDYFe+ANzlcok ODWOIf3aGnLx7IGA4oDUDo8zv1A+hM/8YwhQbn7xouRxk/6jxvrKqb4WpSM6+xPXyDIABO rYlRUzV0m3NsgI1VgdU/r7wXjLnxZSY= Received: from smtp.kernel.org (transwarp.subspace.kernel.org [100.75.92.58]) by dfw.source.kernel.org (Postfix) with ESMTP id 772BF5C4D44; Fri, 18 Oct 2024 19:04:57 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 12FA0C4CEC3; Fri, 18 Oct 2024 19:05:00 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1729278301; bh=FPm25//FWOl+CC1MWzk9hQUI9w5QiPt15s8anu8dklE=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=Q5uvTqpERQM/xf087llem64Geo2ZtGqg+DGlihon2UFTSlgjhC5vcPMah+RtPR5jM V9+ZHf5VFZjTmcJKtPceJvVyS89pTxSHtXENhw0ugAcvlBG6zaDYCdpiePQ9T4NkJw yYakgWahYaH/gaXTAb0ZaK9Vl26Y4TZHqqw28s36BYO6YrF6GxaiOZZWpK4raV87ID 0ysPDESKTrAl8uBRXz//pA6XT++5p9W6jOADfciBCpAsxs9lJEyR+z9SP5o3THMssx tyXLYLe23fAQn89EO4XXbpUekwhntYEad85YicmT7GWnJqiLCAR0TKmHOIvH3/pNMO E7PgKiSEfhCCw== Mime-Version: 1.0 Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=UTF-8 Date: Fri, 18 Oct 2024 22:04:56 +0300 Message-Id: From: "Jarkko Sakkinen" To: "Roberto Sassu" , , , , , Cc: , , , , , , , , , , , , , "Kirill A. Shutemov" , , , "Roberto Sassu" Subject: Re: [PATCH v2] mm: Split critical region in remap_file_pages() and invoke LSMs in between X-Mailer: aerc 0.18.2 References: <20241018161415.3845146-1-roberto.sassu@huaweicloud.com> In-Reply-To: <20241018161415.3845146-1-roberto.sassu@huaweicloud.com> X-Rspam-User: X-Stat-Signature: 7wat15mt4kspfjewy3n8cjgrausdyjor X-Rspamd-Queue-Id: EA327120022 X-Rspamd-Server: rspam02 X-HE-Tag: 1729278287-593661 X-HE-Meta: U2FsdGVkX1/fgh+UUu5Fk3ZZJbsGfASZBT4wq3QFLHzD+YWnm2OxkD1Kd4p+IMqyW64kfecEtamx7YE7+WhzYjh0l06tnMt4UGZDORrGl91aU2K9DQmdeFglUXsYbaPl7a5Xf77P4bXMsfZvwoU5k6RjLccz0mTq6o+o2WGqQkXk9SHKcSFFkTE6gQB3p6fHsLUQNm8llx7W9l2kr9kyqgZ1bvyDkJSsXquyTnIUp0a4JF0Zw0WqYz4obyQ2yv+CLU8LxXqXAJbzBbmdT0B1UIt6TfAmqafvpPPv18R/7/2RB3QdmmJGfbpocxxcVSMZc45jB7vHsVJj0FzvEvNm+7MVKw3UkQ8WKyDA3EL71AUaUdCOLITwqEk9z+vb6A4PUQLENNVbXxrci7ommuRYBsQHtNOAO7F86tnZW809CRD7KcNUV9EH6wjBkr4FRbQo5+BbvYfds9LRYxfy7CGIyDo7jjYL4tQ4AKwB0iyQp84vUh0V4e2V/G8qAgXriwIcR4f2ZhqPCRm3uASdp1kjYh0UV5jjDoH+C6KJYva6dmvHfESnf3DT1wtTOY/n2lS+/x6oc25G8yQI0xFEBEJegGSo8Nrh48XxjrMOGFktbdJ1y5LRnFv3hyssaCZsOUUJooOqj+Opc4Jt8fCpFdmFRsRgJwCGXrJT7SJNm580E2TToZC2U2q9t/6oBV0W6A9Lhm8qLIChuICWj1mBcY/QUXy6tUNRUgUlAcMAALSfbsLA4p+buWhqlIpFJIJmFrg2pPnjog4bB1/gpmQMQnSOQFoXnW/po0GiS3b4P9wTDSiQwC1Yn6tbLKzC459QUSTuhsA/Z8LOaE6qJPGK7DGbPq9ZdaiPBgvVNUucMaxESi0ZcalX4EpIp/R2voAp0yMi9pGB6Mem7MSr3vS28YR162fwzqkd7k0435vRGiaJxK+g2jp1AF98ejaRsI6WLjT4JDNBUbclTmVDGDHauYA pAGTUPKv 8UJeJLD8JiURPBMPgth0zVzJ9Jklb1EPpc3k26dKFnMYpINKQsnkYq1HRqukXD2ny6ANJ7HRvN4jMaCGZqXv6tu+e4W6FeX3zd/99vMa2OcU4RWf09YRddvSNlx6x4JUCWCeWZJUD9aEyNQwVurUW8nY8ti4p97749asg1UyZBqU9hQNazBZSSRylJMZzxim/j0b0nN9s0EwbWySqlXfYCeEM8japyykmpkhvKt2xThV51Lfo1W0JrYeWIpnxjwm0B9EF3R+ZaF5YkycVi1elRMMp9JYHMNkkFJ9VtAJrrvjmJYp1uD8HeL77YRCBo2szIhvLRyGfcIm7bO84arUuk4EIllOZo3B0/lr0XimjyUu4T2ZvZoS12na0zqKXr20g1vBmwONJtRiKu4iJcXiDs+QJUf1nQ4GTM6DcA0WCTpPmUFckLT+MXoyqusI3LTiWzr0KkN0b7rDdNPO+Z0Iezpl1hjL7hn//tjjGtk4sI6cugJwi+4VqeCXjhg+VMFbwXNTrl0O+JKzproWouqTuHI6PuOn8hn4/cVGZmEzgrtPIem77XGol7lHsfMjyQTbG7v1K 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 18, 2024 at 7:14 PM EEST, Roberto Sassu wrote: > From: "Kirill A. Shutemov" > > Commit ea7e2d5e49c0 ("mm: call the security_mmap_file() LSM hook in > remap_file_pages()") fixed a security issue, it added an LSM check when > trying to remap file pages, so that LSMs have the opportunity to evaluate > such action like for other memory operations such as mmap() and mprotect(= ). > > However, that commit called security_mmap_file() inside the mmap_lock loc= k, > while the other calls do it before taking the lock, after commit > 8b3ec6814c83 ("take security_mmap_file() outside of ->mmap_sem"). > > This caused lock inversion issue with IMA which was taking the mmap_lock > and i_mutex lock in the opposite way when the remap_file_pages() system > call was called. > > Solve the issue by splitting the critical region in remap_file_pages() in > two regions: the first takes a read lock of mmap_lock, retrieves the VMA > and the file descriptor associated, and calculates the 'prot' and 'flags' > variables; the second takes a write lock on mmap_lock, checks that the VM= A > flags and the VMA file descriptor are the same as the ones obtained in th= e > first critical region (otherwise the system call fails), and calls > do_mmap(). > > In between, after releasing the read lock and before taking the write loc= k, > call security_mmap_file(), and solve the lock inversion issue. > > Cc: stable@vger.kernel.org # v6.12-rcx > Fixes: ea7e2d5e49c0 ("mm: call the security_mmap_file() LSM hook in remap= _file_pages()") > Reported-by: syzbot+1cd571a672400ef3a930@syzkaller.appspotmail.com > Closes: https://lore.kernel.org/linux-security-module/66f7b10e.050a0220.4= 6d20.0036.GAE@google.com/ > Reviewed-by: Roberto Sassu > Reviewed-by: Jann Horn > Reviewed-by: Lorenzo Stoakes > Tested-by: Roberto Sassu > Tested-by: syzbot+1cd571a672400ef3a930@syzkaller.appspotmail.com > Signed-off-by: Kirill A. Shutemov > --- > mm/mmap.c | 69 +++++++++++++++++++++++++++++++++++++++++-------------- > 1 file changed, 52 insertions(+), 17 deletions(-) > > diff --git a/mm/mmap.c b/mm/mmap.c > index 9c0fb43064b5..f731dd69e162 100644 > --- a/mm/mmap.c > +++ b/mm/mmap.c > @@ -1640,6 +1640,7 @@ SYSCALL_DEFINE5(remap_file_pages, unsigned long, st= art, unsigned long, size, > unsigned long populate =3D 0; > unsigned long ret =3D -EINVAL; > struct file *file; > + vm_flags_t vm_flags; > =20 > pr_warn_once("%s (%d) uses deprecated remap_file_pages() syscall. See D= ocumentation/mm/remap_file_pages.rst.\n", > current->comm, current->pid); > @@ -1656,12 +1657,60 @@ SYSCALL_DEFINE5(remap_file_pages, unsigned long, = start, unsigned long, size, > if (pgoff + (size >> PAGE_SHIFT) < pgoff) > return ret; > =20 > - if (mmap_write_lock_killable(mm)) > + if (mmap_read_lock_killable(mm)) > return -EINTR; > =20 > + /* > + * Look up VMA under read lock first so we can perform the security > + * without holding locks (which can be problematic). We reacquire a > + * write lock later and check nothing changed underneath us. > + */ > vma =3D vma_lookup(mm, start); > =20 > - if (!vma || !(vma->vm_flags & VM_SHARED)) > + if (!vma || !(vma->vm_flags & VM_SHARED)) { > + mmap_read_unlock(mm); > + return -EINVAL; > + } > + > + prot |=3D vma->vm_flags & VM_READ ? PROT_READ : 0; > + prot |=3D vma->vm_flags & VM_WRITE ? PROT_WRITE : 0; > + prot |=3D vma->vm_flags & VM_EXEC ? PROT_EXEC : 0; Not an actual review comment but we don't have a conversion macro and/or inline for this, do we (and opposite direction)? > + > + flags &=3D MAP_NONBLOCK; > + flags |=3D MAP_SHARED | MAP_FIXED | MAP_POPULATE; > + if (vma->vm_flags & VM_LOCKED) > + flags |=3D MAP_LOCKED; > + > + /* Save vm_flags used to calculate prot and flags, and recheck later. *= / > + vm_flags =3D vma->vm_flags; > + file =3D get_file(vma->vm_file); > + > + mmap_read_unlock(mm); > + > + /* Call outside mmap_lock to be consistent with other callers. */ > + ret =3D security_mmap_file(file, prot, flags); > + if (ret) { > + fput(file); > + return ret; > + } > + > + ret =3D -EINVAL; > + > + /* OK security check passed, take write lock + let it rip. */ > + if (mmap_write_lock_killable(mm)) { > + fput(file); > + return -EINTR; > + } > + > + vma =3D vma_lookup(mm, start); > + > + if (!vma) > + goto out; > + > + /* Make sure things didn't change under us. */ > + if (vma->vm_flags !=3D vm_flags) > + goto out; > + if (vma->vm_file !=3D file) > goto out; > =20 > if (start + size > vma->vm_end) { > @@ -1689,25 +1738,11 @@ SYSCALL_DEFINE5(remap_file_pages, unsigned long, = start, unsigned long, size, > goto out; > } > =20 > - prot |=3D vma->vm_flags & VM_READ ? PROT_READ : 0; > - prot |=3D vma->vm_flags & VM_WRITE ? PROT_WRITE : 0; > - prot |=3D vma->vm_flags & VM_EXEC ? PROT_EXEC : 0; > - > - flags &=3D MAP_NONBLOCK; > - flags |=3D MAP_SHARED | MAP_FIXED | MAP_POPULATE; > - if (vma->vm_flags & VM_LOCKED) > - flags |=3D MAP_LOCKED; > - > - file =3D get_file(vma->vm_file); > - ret =3D security_mmap_file(vma->vm_file, prot, flags); > - if (ret) > - goto out_fput; > ret =3D do_mmap(vma->vm_file, start, size, > prot, flags, 0, pgoff, &populate, NULL); > -out_fput: > - fput(file); > out: > mmap_write_unlock(mm); > + fput(file); > if (populate) > mm_populate(ret, populate); > if (!IS_ERR_VALUE(ret)) BR, Jarkko