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 80F20C25B74 for ; Thu, 16 May 2024 06:11:56 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id E545F6B036D; Thu, 16 May 2024 02:11:55 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id E03DF6B0370; Thu, 16 May 2024 02:11:55 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id CA4756B0371; Thu, 16 May 2024 02:11:55 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0011.hostedemail.com [216.40.44.11]) by kanga.kvack.org (Postfix) with ESMTP id A736E6B036D for ; Thu, 16 May 2024 02:11:55 -0400 (EDT) Received: from smtpin10.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay07.hostedemail.com (Postfix) with ESMTP id 39F6B160484 for ; Thu, 16 May 2024 06:11:55 +0000 (UTC) X-FDA: 82123238190.10.A1390F8 Received: from mail-ed1-f50.google.com (mail-ed1-f50.google.com [209.85.208.50]) by imf02.hostedemail.com (Postfix) with ESMTP id 6424C80004 for ; Thu, 16 May 2024 06:11:53 +0000 (UTC) Authentication-Results: imf02.hostedemail.com; dkim=pass header.d=google.com header.s=20230601 header.b=AtukrHfu; spf=pass (imf02.hostedemail.com: domain of jeffxu@google.com designates 209.85.208.50 as permitted sender) smtp.mailfrom=jeffxu@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=1715839913; 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=zwSzN8fvfDv5GDu8auPPWOoq9lZrwSpmFoOwo9DGQxk=; b=fxx3TSCLbCNpUaquVmW0s3gu2+E4FZz9sGtOnq32gVE/LRTMDM6uZeFY4LAI9D9L5NjKTJ 6Vcp4h3gwt0mu0Um/zQzXB+istzo/2k85v7hJuOxjEFOViWhXO8QlwWFdh1jyaaN6CrM4E 2Rq/YYBfEWYQ8GPcwwwq2CYqx3EWyjU= ARC-Authentication-Results: i=1; imf02.hostedemail.com; dkim=pass header.d=google.com header.s=20230601 header.b=AtukrHfu; spf=pass (imf02.hostedemail.com: domain of jeffxu@google.com designates 209.85.208.50 as permitted sender) smtp.mailfrom=jeffxu@google.com; dmarc=pass (policy=reject) header.from=google.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1715839913; a=rsa-sha256; cv=none; b=gliDXWrDtxGZn9xEGSo7JH+Gj05jLsjGsM44vO9lJsPcPGLVp4MDjZ8qNPMIgfu4Ki9yjB MtCbucwJWq6PCYHZIjtNNh0s8zDIi4Xg9UvTin5hYpUlpHeF7lAU37p47UeGkQdal77zJe fjeQcM1aDd3BRrDfmNQ4bt14gNZNAFY= Received: by mail-ed1-f50.google.com with SMTP id 4fb4d7f45d1cf-5750a8737e5so3038a12.0 for ; Wed, 15 May 2024 23:11:53 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20230601; t=1715839912; x=1716444712; darn=kvack.org; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:from:to:cc:subject:date :message-id:reply-to; bh=zwSzN8fvfDv5GDu8auPPWOoq9lZrwSpmFoOwo9DGQxk=; b=AtukrHfuJyBFMQRjPY+0gl04asQa4uwy4+DyxW6ItpgWifSRUyaC/AZppFWK0ZnV3Z 9cxBwsAFdXhM78TC7ZL6RlOvAcFr0E7R17qZWDeI2fnzujpdtvgdPfTHhn8qzwsYlTDH YWRsv9JmqLPt/zjIvLqpVbudFPoG/GtYC3ahcJrJQJYAntycUBBM2KabsHxnqNWW89iA 8iUwBHO4wFfGkdToFMqMhXocfOzcZYfhUBhFO7xIZFbC6BPw4CRvBDno1eUYl3+z68Rb Q37X+AJh3ytNtd6/UyMQGNb+95AYg7pmm80Dc+mmhWxJDs5MkZVsja8Hx6kPbgRLHogd sJPg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1715839912; x=1716444712; h=content-transfer-encoding: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=zwSzN8fvfDv5GDu8auPPWOoq9lZrwSpmFoOwo9DGQxk=; b=q3M9B/wZ+5IYNb/GT87rzRExFUnvYuxhUDquJxZXy7nUpAOMT9I5psQTqHOpYcXhLp U802b/zi6mE6IO/ToSwgg/xwFisg9HHtXQ99i8ETFvUhc4bc81uzNFdz9ZFqKI/PY18A 6mjPOszY3c/ba863mWCuavtC1KEA73y9KmERxu1Cx5Q6+oUyozejNomcMeksDHHMS1pZ 83J6S5D/doAEoY+wE0XfBAB4k96lItfQ0Yr6RehJAmh9c5Mpfkb8ugPhgK8vGyMCys/c 1uLF/NbQJ8V4ufwSd4NfhGvYVWg9pnPytmEkBQhSPaHSZXiJwd+zpqx77boDm9E7A1M1 rfbg== X-Gm-Message-State: AOJu0Yw77ShzkiX+gd9JVPAuL0fQEps20wRgosvIcwBmnoDTvAg0hj0m tJTWcZ4iiSJoDzdGlG+3ZU8CUcCEG4qnFrTlPzceS3JfR/Tawi2xNIz1E2nFjrsDQNhX8tu/P7y paw+GpD1oOS7KFDG5YjIfJs1ZZ4S8FT/lfEvU X-Google-Smtp-Source: AGHT+IEp0x2v9zSQfirOjmV+BT0Ym9aTirvBrBvS0QkHPlchfgN4t9zGFwynf4n0RLKyS+bv4MaASd94Zp7m+NCTcXE= X-Received: by 2002:a05:6402:5248:b0:574:e7e1:35bf with SMTP id 4fb4d7f45d1cf-574e7e13675mr685505a12.7.1715839910575; Wed, 15 May 2024 23:11:50 -0700 (PDT) MIME-Version: 1.0 References: <20240513191544.94754-1-pobrn@protonmail.com> In-Reply-To: <20240513191544.94754-1-pobrn@protonmail.com> From: Jeff Xu Date: Wed, 15 May 2024 23:11:12 -0700 Message-ID: Subject: Re: [PATCH v1] memfd: `MFD_NOEXEC_SEAL` should not imply `MFD_ALLOW_SEALING` To: =?UTF-8?B?QmFybmFiw6FzIFDFkWN6ZQ==?= Cc: linux-mm@kvack.org, linux-kernel@vger.kernel.org, linux-kselftest@vger.kernel.org, akpm@linux-foundation.org, dmitry.torokhov@gmail.com, dverkamp@chromium.org, hughd@google.com, jorgelo@chromium.org, skhan@linuxfoundation.org, keescook@chromium.org Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Stat-Signature: by8wrex3saeth64mx4r489n5fp4yjaij X-Rspamd-Queue-Id: 6424C80004 X-Rspam-User: X-Rspamd-Server: rspam10 X-HE-Tag: 1715839913-609649 X-HE-Meta: U2FsdGVkX19wW9l4Zn9Ir2GF8o3kUWQLjKQfhSn1p0XYG1PUfpZlAtErsnzGOrgMQr9L/8ff5mc4bSM8ssRGHUZ7egumElcol7I/vowl/QN69ssd+og4XRZa8hXISkqeEX0BKAJQ0nt8y/oXwzpuAe8EaihtXDrM5/RWsG0etemuu8c/UvmbN9Syp7g76F3Z3hdoANQpVrpJmL/2XK3SO31EHwiIWdg2t5aZ/3rMm7/medYFOP3KP7wB5vE0eazxYO46+rZI1cFNsF1O481G+Uhl+BMlLKCXvkaz4JeRxVpsa0ndXog2ThigN22+6io2CLeb8Yym8Hng4Alch7vGg/NDvxXbRG7lg/YKq1nk0AfpcX+nrOXPppb0cm8sA97ds2AfdKoSL9Hyl2/JwPBkuNF8okbYVuP3Wr6tUXJz/coopBwi7injmmcDAcnlhLb80rv+F8+MLkZ4WOW9ScUyP9m81Od0WoE0QRTPvpXOY3LchqYrqRV+3OD982IYvNJsQDjDZRBAvqpXvNUUYw1YsSpPDQAw+O/cWt34gYrgYDaykXp/vLH936bp1YyJMWopWHCLDplfvby+6NxCx4VxT2mWPmWsaqan0/lqPqVhqmeI3CRqdgRxqBfgHC9xknEKQemIZqoMkX4kw52B/d/WOg7vKqTAoAcpvsJCYkaKQjDQtiPT41oXog+l/ELtDU367W8MycINTIILuQ2VaWDjjmW8Wh77iIa+6/8vI9ofHVM6dzpZDtXLmhbtZ6bsq5EiOGdvXiWhSGzcLM+pUtSvga8H6VvgzXlAfCXdXuev7DrHycc515TJtTv3+EbRW4mW165pbirVIkKTv66q7XqRqcxfP+kfR17qkYBKp9LhsssBc5lh5NIeyO4+cIdvkNW5s1M4F+WjjUbhw8/BSVgHQtUNJsJyIRBczX18AoLbVeLSO1llmDi5RpsiWbqX+UHu4jdmXTXAa896QqXYf4M uyG+0bRl MQIcec2BCMNItXNDYa46JQIXqNqv4GN6/jGe3Siv+lqOIS9z6vs8QaQvOsJHQrbqufh1MrRS/Cuc+qZnrcJxP+m+Xin/EieDjTHW4MmZIkyBxJJrae0JzaI7kJxCYPjVStesD0bgxErTpOQtuhzPLKubDrUiPix26vcBX8I9+ZRS34jQ6Mu0reV2ZDHIcEK95FN7PHGKyikgC02yXpJldrlOcc+tv3pluE9YcfEXmAkmFSBFEuWQcCGsrQQ+MRVAKGuy0sH+FECiNq5eHEpAX2NKFG//1vjypiokCEqgy7PIgbBwA9WRB2NhwyNro/pLSas2YVJwwroyXfb+Dk7y2yCkchT1oXEQqAwV2A+8nrDWwI/LgPpwcNoytcDcGBm8Of25Cv3litfRJrNNstuks3Z/5a6UQsDiWflIfvq8EPk6unS/oqJia+pM5zeotTXMtqXnOPgyOVYfvu7jZ7cmN0TWy42ggsXFhpweCAn4eGqMwZf4= 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 Mon, May 13, 2024 at 12:15=E2=80=AFPM Barnab=C3=A1s P=C5=91cze wrote: > > `MFD_NOEXEC_SEAL` should remove the executable bits and set > `F_SEAL_EXEC` to prevent further modifications to the executable > bits as per the comment in the uapi header file: > > not executable and sealed to prevent changing to executable > > However, currently, it also unsets `F_SEAL_SEAL`, essentially > acting as a superset of `MFD_ALLOW_SEALING`. Nothing implies > that it should be so, and indeed up until the second version > of the of the patchset[0] that introduced `MFD_EXEC` and > `MFD_NOEXEC_SEAL`, `F_SEAL_SEAL` was not removed, however it > was changed in the third revision of the patchset[1] without > a clear explanation. > > This behaviour is suprising for application developers, > there is no documentation that would reveal that `MFD_NOEXEC_SEAL` > has the additional effect of `MFD_ALLOW_SEALING`. > Ya, I agree that there should be documentation, such as a man page. I will work on that. > So do not remove `F_SEAL_SEAL` when `MFD_NOEXEC_SEAL` is requested. > This is technically an ABI break, but it seems very unlikely that an > application would depend on this behaviour (unless by accident). > > [0]: https://lore.kernel.org/lkml/20220805222126.142525-3-jeffxu@google.c= om/ > [1]: https://lore.kernel.org/lkml/20221202013404.163143-3-jeffxu@google.c= om/ > > Fixes: 105ff5339f498a ("mm/memfd: add MFD_NOEXEC_SEAL and MFD_EXEC") > Signed-off-by: Barnab=C3=A1s P=C5=91cze > --- > > Or did I miss the explanation as to why MFD_NOEXEC_SEAL should > imply MFD_ALLOW_SEALING? If so, please direct me to it and > sorry for the noise. > Previously I might be thinking MFD_NOEXEC_SEAL implies MFD_ALLOW_SEALING because MFD_NOEXEC_SEAL seals F_SEAL_EXEC, and sealing is added only when MFD_ALLOW_SEALING is set. I agree your patch handles this better, e.g. mfd_create(MFD_NOEXEC_SEAL) will have F_SEAL_SEAL and F_SEAL_EXEC mfd_create(MFD_NOEXEC_SEAL|MFD_ALLOW_SEALING) will have F_SEAL_EXEC > --- > mm/memfd.c | 9 ++++----- > tools/testing/selftests/memfd/memfd_test.c | 2 +- > 2 files changed, 5 insertions(+), 6 deletions(-) > > diff --git a/mm/memfd.c b/mm/memfd.c > index 7d8d3ab3fa37..8b7f6afee21d 100644 > --- a/mm/memfd.c > +++ b/mm/memfd.c > @@ -356,12 +356,11 @@ SYSCALL_DEFINE2(memfd_create, > > inode->i_mode &=3D ~0111; > file_seals =3D memfd_file_seals_ptr(file); > - if (file_seals) { > - *file_seals &=3D ~F_SEAL_SEAL; > + if (file_seals) > *file_seals |=3D F_SEAL_EXEC; > - } > - } else if (flags & MFD_ALLOW_SEALING) { > - /* MFD_EXEC and MFD_ALLOW_SEALING are set */ > + } > + > + if (flags & MFD_ALLOW_SEALING) { > file_seals =3D memfd_file_seals_ptr(file); > if (file_seals) > *file_seals &=3D ~F_SEAL_SEAL; > diff --git a/tools/testing/selftests/memfd/memfd_test.c b/tools/testing/s= elftests/memfd/memfd_test.c > index 18f585684e20..b6a7ad68c3c1 100644 > --- a/tools/testing/selftests/memfd/memfd_test.c > +++ b/tools/testing/selftests/memfd/memfd_test.c > @@ -1151,7 +1151,7 @@ static void test_noexec_seal(void) > mfd_def_size, > MFD_CLOEXEC | MFD_NOEXEC_SEAL); > mfd_assert_mode(fd, 0666); > - mfd_assert_has_seals(fd, F_SEAL_EXEC); > + mfd_assert_has_seals(fd, F_SEAL_SEAL | F_SEAL_EXEC); > mfd_fail_chmod(fd, 0777); > close(fd); > } > -- > 2.45.0 > Reviewed-by: Jeff Xu Thanks! -Jeff