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 B9B44C52D7B for ; Tue, 13 Aug 2024 23:21:32 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 4A3356B0085; Tue, 13 Aug 2024 19:21:32 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 42BA76B0088; Tue, 13 Aug 2024 19:21:32 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 2CC276B0089; Tue, 13 Aug 2024 19:21:32 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0010.hostedemail.com [216.40.44.10]) by kanga.kvack.org (Postfix) with ESMTP id 0C1146B0085 for ; Tue, 13 Aug 2024 19:21:32 -0400 (EDT) Received: from smtpin19.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay01.hostedemail.com (Postfix) with ESMTP id 75FC41C38A8 for ; Tue, 13 Aug 2024 23:21:31 +0000 (UTC) X-FDA: 82448795982.19.17DB5EA Received: from mail-pg1-f170.google.com (mail-pg1-f170.google.com [209.85.215.170]) by imf23.hostedemail.com (Postfix) with ESMTP id A8C74140018 for ; Tue, 13 Aug 2024 23:21:29 +0000 (UTC) Authentication-Results: imf23.hostedemail.com; dkim=pass header.d=gmail.com header.s=20230601 header.b=Rqq6p1nb; dmarc=pass (policy=none) header.from=gmail.com; spf=pass (imf23.hostedemail.com: domain of andrii.nakryiko@gmail.com designates 209.85.215.170 as permitted sender) smtp.mailfrom=andrii.nakryiko@gmail.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1723591232; a=rsa-sha256; cv=none; b=ExiVKfD2YE9typbTKXhZKGB4F59gSvKaH0af9PL1QOGyPrFddzE9N/pVXeFCtejZc/3QVq 4IsZ2vYJ9hgDncwXP5/MxovbnzIdvH0ikCJJR5ecoMTgvosxqvJa1qp0jPSrREG9HlNV7w NcpSBLWo2zPmR5WFP5TlGTKLHfVpINc= ARC-Authentication-Results: i=1; imf23.hostedemail.com; dkim=pass header.d=gmail.com header.s=20230601 header.b=Rqq6p1nb; dmarc=pass (policy=none) header.from=gmail.com; spf=pass (imf23.hostedemail.com: domain of andrii.nakryiko@gmail.com designates 209.85.215.170 as permitted sender) smtp.mailfrom=andrii.nakryiko@gmail.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1723591232; 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=JRyVpZs3XdkTAtKOIHKoJQ2IsgMagpAKPl7vl1yKGVg=; b=0Ii0TQ4HtL20g/0ulYpBvltg5lbq0owHUpa44bEM4Y18Ne8SKo3xATfcKMP8VIEyNzbkuZ ahPZoPO/x8YRe5O5tItBP86hUBVgQz+PRPGCE314xJo7CSFSPcRWzM9h8FD2ruDVtgPZFi y9+cJStRoWo7xUywbnOtaxbfvIes2js= Received: by mail-pg1-f170.google.com with SMTP id 41be03b00d2f7-7afd1aeac83so259569a12.0 for ; Tue, 13 Aug 2024 16:21:29 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1723591288; x=1724196088; 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=JRyVpZs3XdkTAtKOIHKoJQ2IsgMagpAKPl7vl1yKGVg=; b=Rqq6p1nb7LinkjBIWr2Ti9vcJHUpKv91EKns5SUhV1dpudEJOAE6WYLozSXYY4aCX7 if2tN/thgdhmEFmKR9umk2eI3YDyMcqFlO5fLekupcNMuCsbOx2jF5pd+hONe3sfh7jC sQMvI6UMO9s8uyfS71zbO7ZrIKhZm5Ub90e1yLfTOuiTEjUvcRLpPOeOUsmFC2SioZS7 mQBnv0S3Mi5XGOeAuPDRZTG3YyAHFHKgKmPUorNrizMdLyGYKnTJqJr/oLC/SI+sDq1y BQA4U32OYpx1jbKImzIRiEyYJg4z/3y+xtKgVLbdtbbX4n6MGiWHkfjS2H6Kmpz4vExA MjBg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1723591288; x=1724196088; 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=JRyVpZs3XdkTAtKOIHKoJQ2IsgMagpAKPl7vl1yKGVg=; b=NsmyZJq9m+s4wSVTX+9opBqKr20KXWCiwkeP3yR6N2ytotLXaNLFAx51JYRqRRegZO ITkq8kzTTi3sVM9f52Z/behiU3MSXeyZfxuojJRYg2I9j7l4xGfF6bpY/6Jcqk/KDTor PzmIwthTVVTQEKKxdnevgVmtaCsrMpcOnVwquLyxidmpUPjJjexz15scCoM4NkhS8stR FqkoEIvIvuDP58bNteQW7bXN4KJDZjkY57PmX6GjOOsSv9raHCtvxenoCIpePlrNtnsT 9ShJCC4WwUrs65M8DiwF25VgUK/a0NoEfofQuaEk0xmY5TtijQ2BWZ49slFqS98jtWPL QrWg== X-Forwarded-Encrypted: i=1; AJvYcCW7wjf48YhpHydsNXa7Umlr0qfFBhHrpax78/wypL8fzx8AfnbZ0D8JdToIMxoS/5XCF4R584VKsOSYQh2mVGJC794= X-Gm-Message-State: AOJu0Yzz99ILRVyku8NkOMA8zpKQ5LsBuGyFJ5/cof4OHqKSNJd81AMC 4oK3+nNEBTV1aJkID/9u/p9HTBi2UNQn78Xk3d421dvC/k194fXHg95n0wp2R2RF9pquS/dAqr2 UJxbF0xAvQdnmmWO8AKTN7/RDiR8= X-Google-Smtp-Source: AGHT+IFPgLZng5y94JxpAKfjZfHPUVopiPIVhjrWL7lyUUQAEEn+X2FVV/76vMV3lqN11qMFZg0DDadF+clCNCCF8Bo= X-Received: by 2002:a17:90a:474c:b0:2c9:63d3:1f20 with SMTP id 98e67ed59e1d1-2d3ace55317mr433766a91.18.1723591288215; Tue, 13 Aug 2024 16:21:28 -0700 (PDT) MIME-Version: 1.0 References: <20240813002932.3373935-1-andrii@kernel.org> <20240813002932.3373935-2-andrii@kernel.org> In-Reply-To: From: Andrii Nakryiko Date: Tue, 13 Aug 2024 16:21:16 -0700 Message-ID: Subject: Re: [PATCH v5 bpf-next 01/10] lib/buildid: harden build ID parsing logic To: Jann Horn Cc: Andrii Nakryiko , bpf@vger.kernel.org, linux-mm@kvack.org, akpm@linux-foundation.org, adobriyan@gmail.com, shakeel.butt@linux.dev, hannes@cmpxchg.org, ak@linux.intel.com, osandov@osandov.com, song@kernel.org, linux-fsdevel@vger.kernel.org, willy@infradead.org, stable@vger.kernel.org Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Rspamd-Server: rspam12 X-Rspamd-Queue-Id: A8C74140018 X-Stat-Signature: 7hsykzf76xmo96oatuxjab5338t3oxwy X-Rspam-User: X-HE-Tag: 1723591289-54245 X-HE-Meta: U2FsdGVkX19ouXV99vVloVX+qDAL11ED3RQPiBsLzWWS7KBHztiIBXX9UE0NIBemtQ0PSToUzVJ3eKuma+lHZeCVmn7r1wkqukpCF5dZ5XcqLVRzDKqIU7fBULQQZ/ePXrWc9N3uKLFZBOWnEs2RIfmMfMMcstrwNE+OrJcCJ/eq/hsLP/F7gIaNQ2+ov3HzYGu6SSfjfD0noeMVZU8rMNRz8mdjnrWWTquxuaclKo5x1KotdY9iiJakjwnSTE0023aMa3eOujDC1DpNHz4s9VtkqBw5ihvjDsDL4GFUfWMD1TPWfG0fapmWk6gb2bBWqABML4bboOemJp82Ah6AusXz7njRD2pc8FGmBmT+enMp/N5oblPBCQLfzCjIHPu/2zSbF9jQzr+fj/7JF4vn2+eFxF7p9RT0k8goHgcb+UjSRy87mFkZBMfSAeKkgnIS4/NR1ZA1UT9tq+PKF4VSfS9oRfHBwvf+jP/hVZcc8ruoHyDGmxSXmOhWn/jMqWrchHu6kUBfZvnZUFQnTdnE/XDpdSg1/8lHQfe04/2uunufbrPa/qzfgIouZzeKZtbaxvxkypFKFmBMc/f90jWk1c8Y5FL8FrqNet7xX3cmhlpiX2WQ5YpEkQ535/bzoP/JCfM4AlWuHNIjcCQCKN8bRBUU+9eSpU1Yz6HuZxzzRZd7A0o3GmyGn5zO4z3RZxrj+oFnsuavu3WXHfh0nk2JILDhYl9SckuatHhOa4yqRwuwL56dmArUzLEssDipkrb3sdeJqAgCkMYsmIQ18DYKpn+Yh4PSvN3rB0OW6DZ1WomSJO4iIrewkFlTaGakUQ2WqD56xgYAcVqsHVQJTLTSZMRytu6x94p7faL/NwSY3x4GCy9OxnfyZtuAim6Pr44eNYpLQmWsQjq4q8qfOdaUAPpeWtRSeDFIdQ2wtu45CRrZmd+W3BO/yXgYhM9/B01VPVbixuWkfPO2EhcQ2mS ebMLzvmJ /y7AlkKNJr2b7eGKaVJfbZDwzdvQ61oI2Loj6LrOw0dLI+Aq3UALwbKqPSojCzaz0gUWwjF+5Y7ag58bEXcpVqoaDnNEFNnZ3Lp03eO2/AgveFqSwPtB4M7oZZFY8832qAf/LL5+/JQV1KzeGUWnlL1SY9vPf3GR9Jg6aigYIqS3l7Gg/wVov09Aol784GETBY01hS8i24jAJZiy4bqpxT+d+mjBUrESHoVxqWFd0QlaZWN5lxHrWMzMHS1vKoJn94K979xj2OQsv/OionzrH84USjZKEM5+wqg3/rz3SgqjMQNQRBngFTIr8j1tgM985pYmg0oX/VMRNH9m25n1cBnP0DV2M/+0oCBGxolIjydVxcIiuYTv96TQSkD/FcRfux3XEgAOPddBmyve1QWAtu6fN2zyCJhMfsiyu 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 Tue, Aug 13, 2024 at 1:59=E2=80=AFPM Jann Horn wrote: > > On Tue, Aug 13, 2024 at 2:29=E2=80=AFAM Andrii Nakryiko wrote: > > Harden build ID parsing logic, adding explicit READ_ONCE() where it's > > important to have a consistent value read and validated just once. > > > > Also, as pointed out by Andi Kleen, we need to make sure that entire EL= F > > note is within a page bounds, so move the overflow check up and add an > > extra note_size boundaries validation. > > > > Fixes tag below points to the code that moved this code into > > lib/buildid.c, and then subsequently was used in perf subsystem, making > > this code exposed to perf_event_open() users in v5.12+. > > Sorry, I missed some things in previous review rounds: > > [...] > > @@ -18,31 +18,37 @@ static int parse_build_id_buf(unsigned char *build_= id, > [...] > > if (nhdr->n_type =3D=3D BUILD_ID && > > - nhdr->n_namesz =3D=3D sizeof("GNU") && > > - !strcmp((char *)(nhdr + 1), "GNU") && > > - nhdr->n_descsz > 0 && > > - nhdr->n_descsz <=3D BUILD_ID_SIZE_MAX) { > > - memcpy(build_id, > > - note_start + note_offs + > > - ALIGN(sizeof("GNU"), 4) + sizeof(Elf32_N= hdr), > > - nhdr->n_descsz); > > - memset(build_id + nhdr->n_descsz, 0, > > - BUILD_ID_SIZE_MAX - nhdr->n_descsz); > > + name_sz =3D=3D note_name_sz && > > + strcmp((char *)(nhdr + 1), note_name) =3D=3D 0 && > > Please change this to something like "memcmp((char *)(nhdr + 1), > note_name, note_name_sz) =3D=3D 0" to ensure that we can't run off the en= d > of the page if there are no null bytes in the rest of the page. I did switch this to strncmp() at some earlier point, but then realized that there is no point because note_name is controlled by us and will ensure there is a zero at byte (note_name_sz - 1). So I don't think memcmp() buys us anything. > > [...] > > @@ -90,8 +97,8 @@ static int get_build_id_32(const void *page_addr, uns= igned char *build_id, > > for (i =3D 0; i < ehdr->e_phnum; ++i) { > > Please change this to "for (i =3D 0; i < phnum; ++i) {" like in the > 64-bit version. This did slip through, yep. I'll check with BPF maintainers if this can be fixed up while applying or whether I should send another revision. > > With these two changes applied: > > Reviewed-by: Jann Horn