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 735BEC2BD09 for ; Fri, 28 Jun 2024 16:36:41 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id BBF086B0085; Fri, 28 Jun 2024 12:36:40 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id B6F766B0088; Fri, 28 Jun 2024 12:36:40 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id A5E1F6B008A; Fri, 28 Jun 2024 12:36:40 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0014.hostedemail.com [216.40.44.14]) by kanga.kvack.org (Postfix) with ESMTP id 866D96B0085 for ; Fri, 28 Jun 2024 12:36:40 -0400 (EDT) Received: from smtpin13.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay08.hostedemail.com (Postfix) with ESMTP id 18EBE1405C1 for ; Fri, 28 Jun 2024 16:36:40 +0000 (UTC) X-FDA: 82280850960.13.8675E87 Received: from mail-pg1-f182.google.com (mail-pg1-f182.google.com [209.85.215.182]) by imf18.hostedemail.com (Postfix) with ESMTP id 309A81C0005 for ; Fri, 28 Jun 2024 16:36:37 +0000 (UTC) Authentication-Results: imf18.hostedemail.com; dkim=pass header.d=gmail.com header.s=20230601 header.b=fyMywG96; spf=pass (imf18.hostedemail.com: domain of andrii.nakryiko@gmail.com designates 209.85.215.182 as permitted sender) smtp.mailfrom=andrii.nakryiko@gmail.com; dmarc=pass (policy=none) header.from=gmail.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1719592580; 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=jGnPoWdruisWSPdQgdSF6ZHXGVwHt0h1jI18T4eJdgs=; b=xX1DmuUkBiAnrn2jrG8uFYx7A2aBssPpdPlec7dRE19X8yCP2UaDf1O83YcOZtixpuA1mK SWiJKfEstoERkLpNT8UlYKhx/BTC1/WR2MTdrP7eHi0vIFRiKnjJeWf3hGgf7B91/yEvWO Ah0vAZ6tUFo1Uk+w4P29uZz0snDDPdc= ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1719592580; a=rsa-sha256; cv=none; b=lkG/IlK1QLL6orlnj0bjTReI03JkxtOkYLF3XXYpsIhGgppStxc2i6qCkPC5eZUnar7ZOe fu+4N8EuIl4/T1exm/SXtyMIAXYRZvO37h/jHaUaAcmDpSvxBOeEPqGAlhRBrRM/hjMs7A H4bxDNSysc9V9sTjhQKZvegA8z0QLMI= ARC-Authentication-Results: i=1; imf18.hostedemail.com; dkim=pass header.d=gmail.com header.s=20230601 header.b=fyMywG96; spf=pass (imf18.hostedemail.com: domain of andrii.nakryiko@gmail.com designates 209.85.215.182 as permitted sender) smtp.mailfrom=andrii.nakryiko@gmail.com; dmarc=pass (policy=none) header.from=gmail.com Received: by mail-pg1-f182.google.com with SMTP id 41be03b00d2f7-7178ba1c24bso563641a12.3 for ; Fri, 28 Jun 2024 09:36:37 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1719592597; x=1720197397; 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=jGnPoWdruisWSPdQgdSF6ZHXGVwHt0h1jI18T4eJdgs=; b=fyMywG96B/EHJyGTc3+ZFqVG/x+ysydUfeUDuQXJgc9a/UyZpkpc02szeVepdq/yR1 ahxe+fGEi4FCX2OksfQCDmd9HU4N5bPW1Mxn4M212ipaqOFDib5NAYs3yIY84hyRkqGF wXPbJavS1JK+/7gxeIcy+zpb63akQrpKU/OiivY21leVuqIr8z+9ZZvw+VyaNS62aIf3 Th3zl2yCVaxEROK2MUeD6MqFoKO62iIxg9LE9bkPvO9hZkxUzQn3/1qNeVgg3ZTv5TRr fh378eTXgb+PK6PARy2Qbgem5Wr0CmEZT6yJ8Vc+fDYt3+pG1guR7kcZ/C1Qjvl2pgO8 m7xQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1719592597; x=1720197397; 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=jGnPoWdruisWSPdQgdSF6ZHXGVwHt0h1jI18T4eJdgs=; b=Fo8ueSG6h4LSdBHC67d8z00rE7NC+s4yvoT09XH8MWsLaIiJe5jJClXIGQ1NmnbR0t Ued/Nx13cNWb3203O+JZfLhaZDzbXFhQkHPAi+H7+CUmNjF7gqzg718CgKdojQdWxW2D 6wEKl5vYPIjt+4v5vr2JExCwYD46MAJllbY0+VVOqS6xHPfzbKu2iL6fLJsUqzSn/zTZ JGBSaBCSAuaEl/1INQ6mORyEj8zx4xCi8Bb7CL4oB4uBU/RuR1TF/iNfvB8DoZJMiE5l mM522PK1Js5FCr9z4/V0k2V0tU5ScKmJXGdhkT173EZ1/lKwGytYj6mRvScGTcT/sls6 Tzbg== X-Forwarded-Encrypted: i=1; AJvYcCUnhXeRG3BNVm7ZDVuqx5MiFhzR0KSLxIo2LOKMxJmn1rs3VIfCHknWJ/IAf0Co3QtNWVS59mZWSrGPDlnSUM7ag8Y= X-Gm-Message-State: AOJu0YyDyaP5+ronHoqT033/E8aJ+1x9DHI+HOL6zZin0TgPbm/TU9e6 0JnKUhDJ2Pp0CxLC5HoXOyez3Tnk+fL45gl1xdMegTIML2Q6LacFuN4KKBXZAM/nMqUoq8Z0+ye HrmkZZOMSKiupYIgjYHYF1m20FyA= X-Google-Smtp-Source: AGHT+IGWWJNyo0OOO0+HVTZ4yFxBFpAEh7kWeeBff+lMtE0x+3GvMgNVOp0ebiUYq9oTJECmTT22XZAYsJ0gVJy0I9o= X-Received: by 2002:a17:90b:50c8:b0:2c0:238c:4ee6 with SMTP id 98e67ed59e1d1-2c86121446cmr16134737a91.2.1719592596790; Fri, 28 Jun 2024 09:36:36 -0700 (PDT) MIME-Version: 1.0 References: <20240627170900.1672542-1-andrii@kernel.org> <20240627170900.1672542-4-andrii@kernel.org> <878qyqyorq.fsf@linux.intel.com> In-Reply-To: <878qyqyorq.fsf@linux.intel.com> From: Andrii Nakryiko Date: Fri, 28 Jun 2024 09:36:25 -0700 Message-ID: Subject: Re: [PATCH v6 3/6] fs/procfs: add build ID fetching to PROCMAP_QUERY API To: Andi Kleen Cc: Andrii Nakryiko , linux-fsdevel@vger.kernel.org, brauner@kernel.org, viro@zeniv.linux.org.uk, akpm@linux-foundation.org, linux-kernel@vger.kernel.org, bpf@vger.kernel.org, gregkh@linuxfoundation.org, linux-mm@kvack.org, liam.howlett@oracle.com, surenb@google.com, rppt@kernel.org, adobriyan@gmail.com Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Rspamd-Queue-Id: 309A81C0005 X-Stat-Signature: mtwkxon66tdrzgfqaoqa1y4c9foseqbb X-Rspamd-Server: rspam09 X-Rspam-User: X-HE-Tag: 1719592597-615694 X-HE-Meta: U2FsdGVkX1+ZN4PiK4sBKNs2jRTLUk7NTn5zuVvH43RA28eNFE4WnKA0p4co06C9voBTD8r3qWeiwtSPEdZBR2v0FTZnrBNnAKrOyTDtcHkMEE9TW/4gYeoLl/C/2LfTWC2aUpf0yYg0x31e9QaULl5MQg1utQgAnbvX7TKisw2RsLYLFuRjr0D7cHbfcaGOgis2XhU6+3QXiIwNZrndbWPLbrXoOicvoq4ZAp3Cyx6w5XYFnN7vYgd6VUZ1cE4T1GYcWt+xH8yFIFesru28uhB58+gYYX5C9jWmr15ofS3OxD4FVhWluBmUZ+G/8LWKxI8dikPpNkdlkxf1UStecKfrs4ZnGW35I5TScjrZe3bU2Zv8iIaf2h72qIrKZRiq4Hfg7trVK8/VtPSNg/O2PMVDezfGhmAmCmLW0+wQUm52dupk+AeX1Z0E5oxScckgk2jeHk6B5UJg8ONBsSz2ynKn2LM+BBkxJctbFVAL8Cs2+r/7w9s4tjbfZ1GZ7nD8i0SMv/psdcR7wxfYVAqSDzeSeCTe8Q+AfgCP3t6TSLDpHDOzh3cUagP1Bjjj6sLKVsqztbSYzVw8efKH4YvnZ13v7hJodNFIWW13nQkdCTY87BishKxlYUxsnPE34wCshZTBXE36WloHWlwKnZx7aL91kvikSKu03J7LufYp4wmvBKt7hPKcZhcsQ6PX6yYzvXR1MfRNPACCSnGWLMEkWXJZT/XBkUSycYjuvE2cKpEAby5a8oqIs8MDgzCuM4QnJ/dRemwGIYkRYnftIYnW+vY4X/AbQ/w291xemw6GkanY0nsV317jZ51VISe7mi/G3O4tnXh+ffNOZ/i9EjI1OikQAW+fT/BfbT1SGDoR5gNGbLPaxhVBh+t6BuutP2NZO9Wr8x3XdVcaPL/qQPBaZQSa8uKqqLoxTiTp43VnpRkP9RpZpow6on6eeLo8aOqx+DmKT4YepCrz91Aws6O u4oe1KdR CzXcgwGGwzUbtBtt4JbMfGYk3QxlIPTQ5LnePZ09EitjAmD0Beelq0vDOExfpxUI2mFfTgyLyhWddoWPc9lzwvp1Qwt7bJ1IhUIxPVc45NNJ8yuIbwNE+KwnnxGonmG2YirAUh8HyI7NSzFOXsas0DuvMXFDoTDR+h7e3G4GbEBLwWsHutpWHvoCR6uzlwIIf7qIcnydAn9ax3k6eEV784kUG0TsiG9iDZX5JzwQAF/ocFaxVItW+neVrm9iyJE3jo5n1YAAMj+XZy6X4zGle0zshm1zyWfIPGLRte6g9Y17DVaY4eFC8vA9RBNDDHs4WKBbnfpFTGqraF/LUFsLGE4JDKKz2XBw1ZublD8UiFr8ZWwS5Mru3rRbCRR56dUx7uKqz0OOa80gtSGgpvDrZOZyuSpIxicOshtkP8RrTiMPqM7ZWxPI7aTC81wFPMomqIG6RHYxnoqVQkNk= 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 Thu, Jun 27, 2024 at 4:00=E2=80=AFPM Andi Kleen wro= te: > > Andrii Nakryiko writes: > > > The need to get ELF build ID reliably is an important aspect when > > dealing with profiling and stack trace symbolization, and > > /proc//maps textual representation doesn't help with this. > > > > To get backing file's ELF build ID, application has to first resolve > > VMA, then use it's start/end address range to follow a special > > /proc//map_files/- symlink to open the ELF file (this > > is necessary because backing file might have been removed from the disk > > or was already replaced with another binary in the same file path. > > > > Such approach, beyond just adding complexity of having to do a bunch of > > extra work, has extra security implications. Because application opens > > underlying ELF file and needs read access to its entire contents (as fa= r > > as kernel is concerned), kernel puts additional capable() checks on > > following /proc//map_files/- symlink. And that makes > > sense in general. > > I was curious about this statement. It has still certainly potential > for side channels e.g. for files that are execute only, or with > some other special protection. > > But actually just looking at the parsing code it seems to fail basic > TOCTTOU rules, and since you don't check if the VMA mapping is executable > (I think), so there's no EBUSY checking for writes, it likely is exploita= ble. > > > /* only supports phdr that fits in one page */ > if (ehdr->e_phnum > > (PAGE_SIZE - sizeof(Elf64_Ehdr)) / sizeof(Elf64_Phdr)) > <---------- check in memory > return -EINVAL; > > phdr =3D (Elf64_Phdr *)(page_addr + sizeof(Elf64_Ehdr)); > > <---- but page is shared in the page cache. So if anybody manages to map > it for write > > > for (i =3D 0; i < ehdr->e_phnum; ++i) { <----- this loop can go > off into the next page. > if (phdr[i].p_type =3D=3D PT_NOTE && > !parse_build_id(page_addr, bu= ild_id, size, > page_addr + p= hdr[i].p_offset, > phdr[i].p_fil= esz)) > = return 0; > > Here's an untested patch > > Yep, makes sense. I'm currently reworking this whole lib/buildid.c implementation to remove all the restrictions on data being in the first page only, and making it work in a faultable context more reliably. I can audit the code for TOCTOU issues and incorporate your feedback. I'll probably post the patch set next week, will cc you as well. > diff --git a/lib/buildid.c b/lib/buildid.c > index 7954dd92e36c..6c022fcd03ec 100644 > --- a/lib/buildid.c > +++ b/lib/buildid.c > @@ -72,19 +72,20 @@ static int get_build_id_32(const void *page_addr, uns= igned char *build_id, > Elf32_Ehdr *ehdr =3D (Elf32_Ehdr *)page_addr; > Elf32_Phdr *phdr; > int i; > + unsigned phnum =3D READ_ONCE(ehdr->e_phnum); > > /* only supports phdr that fits in one page */ > - if (ehdr->e_phnum > > + if (phnum > > (PAGE_SIZE - sizeof(Elf32_Ehdr)) / sizeof(Elf32_Phdr)) > return -EINVAL; > > phdr =3D (Elf32_Phdr *)(page_addr + sizeof(Elf32_Ehdr)); > > - for (i =3D 0; i < ehdr->e_phnum; ++i) { > + for (i =3D 0; i < phnum; ++i) { > if (phdr[i].p_type =3D=3D PT_NOTE && > !parse_build_id(page_addr, build_id, size, > page_addr + phdr[i].p_offset, > - phdr[i].p_filesz)) > + READ_ONCE(phdr[i].p_filesz))) > return 0; > } > return -EINVAL; > @@ -97,15 +98,16 @@ static int get_build_id_64(const void *page_addr, uns= igned char *build_id, > Elf64_Ehdr *ehdr =3D (Elf64_Ehdr *)page_addr; > Elf64_Phdr *phdr; > int i; > + unsigned phnum =3D READ_ONCE(ehdr->e_phnum); > > /* only supports phdr that fits in one page */ > - if (ehdr->e_phnum > > + if (phnum > > (PAGE_SIZE - sizeof(Elf64_Ehdr)) / sizeof(Elf64_Phdr)) > return -EINVAL; > > phdr =3D (Elf64_Phdr *)(page_addr + sizeof(Elf64_Ehdr)); > > - for (i =3D 0; i < ehdr->e_phnum; ++i) { > + for (i =3D 0; i < phnum; ++i) { > if (phdr[i].p_type =3D=3D PT_NOTE && > !parse_build_id(page_addr, build_id, size, > page_addr + phdr[i].p_offset,