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 B44B2C52D7B for ; Wed, 14 Aug 2024 17:06:52 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 48C716B0089; Wed, 14 Aug 2024 13:06:52 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 43DA06B0092; Wed, 14 Aug 2024 13:06:52 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 2DD296B0095; Wed, 14 Aug 2024 13:06:52 -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 0FD3A6B0089 for ; Wed, 14 Aug 2024 13:06:52 -0400 (EDT) Received: from smtpin19.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay08.hostedemail.com (Postfix) with ESMTP id AB5A7141082 for ; Wed, 14 Aug 2024 17:06:51 +0000 (UTC) X-FDA: 82451480622.19.B546856 Received: from mail-pj1-f50.google.com (mail-pj1-f50.google.com [209.85.216.50]) by imf28.hostedemail.com (Postfix) with ESMTP id A20C4C0035 for ; Wed, 14 Aug 2024 17:06:49 +0000 (UTC) Authentication-Results: imf28.hostedemail.com; dkim=pass header.d=gmail.com header.s=20230601 header.b=aiTUyPGk; spf=pass (imf28.hostedemail.com: domain of andrii.nakryiko@gmail.com designates 209.85.216.50 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=1723655138; 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=eJyDGOa9e890VyGR7m4SBs+qeXxTLNl9Z80pbaWVlzk=; b=IlHoioC3Q3lScSwKBUW1y191aALJI6uy95IDpVXA46B+2TGF37iBUIvlzXG9/S7pMa4hBq y5BhFdtHsohhM7vez6kYtFwuq/aAqbQJWHtoa+5u87HfduwW7CaOATGilr/MM7Kc0PGSWY nSZ2aGmTUYh5EqJihUxrbCcfzQVbeKA= ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1723655138; a=rsa-sha256; cv=none; b=T/Fh3VZitNVAXULmguitJSpEAToFSlhKw2c8trRagH8GkueTzAD0UT+zbGxWtbWx0A1rDX Hm29x8NhhMnpsdl9UHK+4oXFdIYdpnjXmb5W9owaZnl2wGhhYu+WGKVWN+oUl/tWac9AjP oopiW3x6uxsrDKlyMQ/Eit8KSc20tZo= ARC-Authentication-Results: i=1; imf28.hostedemail.com; dkim=pass header.d=gmail.com header.s=20230601 header.b=aiTUyPGk; spf=pass (imf28.hostedemail.com: domain of andrii.nakryiko@gmail.com designates 209.85.216.50 as permitted sender) smtp.mailfrom=andrii.nakryiko@gmail.com; dmarc=pass (policy=none) header.from=gmail.com Received: by mail-pj1-f50.google.com with SMTP id 98e67ed59e1d1-2d3c08541cdso230872a91.2 for ; Wed, 14 Aug 2024 10:06:49 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1723655208; x=1724260008; 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=eJyDGOa9e890VyGR7m4SBs+qeXxTLNl9Z80pbaWVlzk=; b=aiTUyPGkCJcVqRzLBQ04Tq4xGklScla7a7v0UzVUfjep3I60t8Ag7Imi8PNTmZWHBb I8bx6m2SrbIQLMsr2WuBS3f4hSYiFzvaGBrSgn72wKUS5F+5U9nqa6ReQUVoiqS3de6C mSWByIBsEy/JnZpypwhyAt6Kl+Uq/3f0NkFuYAFbzTLlLC79PIijPR4r9h7W2rCBEddc 6cfp0QBytkBtUCLmxItWOj9FDq4w4cW/ohGJvGTMjjAyLVYEfytnS6cJCt0/rB9HjpQl 3JbL+nzY1GP/gDOKTcBBwbD7IE0sFwMkZKjpkaRugWT49QdKWtA1+xka3V+dGEybXl2D XiQw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1723655208; x=1724260008; 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=eJyDGOa9e890VyGR7m4SBs+qeXxTLNl9Z80pbaWVlzk=; b=MyXpLvizmWb2aCWuaC+5jyYcksimI+Q4yYiFMSns4XxEBX+UZZphIWjgPsRUPpFTwr Q4aPD0U68ujuPbPz4OWh6EuKAxh6BvraPchy66x2wvFrcouXiTz/Ip4WNb1CcjZT32Xz E9k8HkBh2e98iqKVez03hdzudOrAWsM4p0nafxKejrEEU0ZMbehXvjFOGefJmQCo7Wku 1Y3Oxj2XDskxBoRRGMeg54QJGpruSfJrd7V6IQ/0LojqY0HjrH4dmRvD44DH4cJQ8it+ dkX07j49MhstrWFhTW5+qFl7XoK1BAjSQGHgFEdopK9NiDzUlVSRSFIt7wueQr8+Tfmy RHCQ== X-Forwarded-Encrypted: i=1; AJvYcCVKr0YU5MaRJiCOZNc/SvfMJNxjSykw7Qk/aDw2agtM+Oqn2CXoqT5Y4TQrb1PgGl9gXQV5e/doASOeAa33NkVDISQ= X-Gm-Message-State: AOJu0YyvsDwmqjZefffNSQr06hXB9hkPCnqFC7UROgGWcLVIC85Orqpl CqRtLmfkO40tiDfm/g4ePxV9POy0ZKBV6OxiTTaV2lM/x/veGMMgB3u41GEq3Jzy9V3nRE5YP/+ NEl/yZJTtv/aTANWEW9wm5l3kGjo= X-Google-Smtp-Source: AGHT+IFpxEFJerNQMdeR4Yo2U8OoEA6DbLNV577OSQZ/EXCkbDWO6HwvzyTFK0iPB1fUKU8OpDK7ocp4RpFKERZ05Gw= X-Received: by 2002:a17:90a:9f97:b0:2c7:ab00:f605 with SMTP id 98e67ed59e1d1-2d3aab43bb0mr3709604a91.20.1723655208070; Wed, 14 Aug 2024 10:06:48 -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: Wed, 14 Aug 2024 10:06:35 -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-Queue-Id: A20C4C0035 X-Stat-Signature: 4uogprumaik63hp7ao35swqqyfzhbq1y X-Rspamd-Server: rspam09 X-Rspam-User: X-HE-Tag: 1723655209-974831 X-HE-Meta: U2FsdGVkX1821M+s8zKdVrZGRtnk4c1FbKjcn7jaM4SU7SxS9+/VlEsgS9ujEsLWjhn9TUffq0S637v/nzwpe5h8fm0Am31l596ySEGsPSlNkWfa46qvAZJLUmWKy5RlAsZHzVvykD+Lp69VytvQuL8i3KxUW+IraFE2MxqWEImaDsIn+4zWQ9uGF4+W2IVlsesn0ZNTxitcF+P6b/7a4BfGBoqQ/fAC57xo3QNPFJMo8vN2uvqj2ckA2Ru8Vvaj4ybOMvRrFrH7zRS5xh8uqMNkYv9r/IHO+APo6iUAoXt7WENFnSbM3obJ2TsxJxpITpWjxxL90hbKKpy8OL23Gydg5MyjTFoeo6xsfD6cjjQpwZsz2F0gmZHnDT5U+g6Qb197a7LVOcXQ9S+UFAIhCJC7Jd1FUMLfyPe9QLszH0WdHeWWZlJHAf5mnQCqdYr4ti17/+oO1qLBfhVFHNUYVvO0lzILwIA+nF9kWJEbiGvR5alwogfrBGTQZJShvJ+gJcKOHtsk5L6gOETyqOyYXCwuSEL3Pxe5q6Y2OCarxhia37gFP61dZPoJKqegvxefuhmuGGWoqC6UkAVX9kycFu2/YleOWnQUKw6NH75AOz1F29BGZq8DnIl3etJbP+gA+JoHkXomgE4iU1KvUtbTMfgz3XpFhkWzSxO0t3cPvLe/+MWfk0l8tY4hDVAZjJmb+9XVKW72FI93fZNBPMQkjWe5tUG0rCE5cwZ0s00Z5VtcsP1nXzDJvxW9NO5lMqDvK4akHhaiTUfECNM2I251pCLQWtywnTwv+H5+IPrF7770nR7ovRzO+ZSLaEixNRrdskDF4FtevakBtkiQI6704H3TGHapDJjuD/DqnrQiPvtJp0+i7BPTKRQUrajQNhtJZhLYi0JVciEFaxuiHy9I1hbFoapUfTBtsGOO3I/NLZ18YQszxlXGcXqLwRjJWpaWM7wyJEg88W2QnS+BhyQ /moEZzih JAoVTujuA9H5CJ2XUJ8fggIdi5ru9qucFFafHDDJR83q6GMaDxGnJnzczo4tC8H9aOnUg3VUqpYQHE2CBDIBjK9OZM0T+n8h6MCdVjE4v+WB41+kpMF9IFRAnAiMFgylaZcJAcm/6GLJLPvVJe66aD7eG8+resgb4NZs2SzOcNoUY8R1jABCFKqX4r+5UjsSxT4W/QLWp7KcG7ocpXGKFCC1b2zEpZRSTtd9kFpuqX2UyK3RPLq4phUC0v2sUnsOnJQgwrnKuOko4S/Ugswz3AitAdfp8mQIbqPkOmMDxeAv0bhiQtZvpjFHA0qUkiC8eIUJprrrVSxM7xa2yTGyRfv1CTeExK7NJt8qLScsWA+e9iVdJc9vC7UiXYKd6f/aEZbyy24AMcUOTLkp1ZvUfxwKuxu8bVKrnJWvxJNYDp4Ic9HY+3aU0Sv09Ww== 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 Wed, Aug 14, 2024 at 9:14=E2=80=AFAM Jann Horn wrote: > > On Wed, Aug 14, 2024 at 1:21=E2=80=AFAM Andrii Nakryiko > wrote: > > On Tue, Aug 13, 2024 at 1:59=E2=80=AFPM Jann Horn wr= ote: > > > > > > 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 entir= e ELF > > > > 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, ma= king > > > > 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 *bu= ild_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(Elf= 32_Nhdr), > > > > - 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 th= e end > > > 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. > > There are two reasons why using strcmp() here makes me uneasy. > > > First: We're still operating on shared memory that can concurrently chang= e. > > Let's say strcmp is implemented like this, this is the generic C > implementation in the kernel (which I think is the implementation > that's used for x86-64): > > int strcmp(const char *cs, const char *ct) > { > unsigned char c1, c2; > > while (1) { > c1 =3D *cs++; > c2 =3D *ct++; > if (c1 !=3D c2) > return c1 < c2 ? -1 : 1; > if (!c1) > break; > } > return 0; > } > > No READ_ONCE() or anything like that - it's not designed for being > used on concurrently changing memory. > > And let's say you call it like strcmp(, "GNU"), and > we're now in the fourth iteration. If the compiler decides to re-fetch > the value of "c1" from memory for each of the two conditions, then it > could be that the "if (c1 !=3D c2)" sees c1=3D'\0' and c2=3D'\0', so the > condition evaluates as false; but then at the "if (!c1)", the value in > memory changed, and we see c1=3D'A'. So now in the next round, we'll be > accessing out-of-bounds memory behind the 4-byte string constant > "GNU". > > So I don't think strcmp() on memory that can concurrently change is allow= ed. > > (It actually seems like the generic memcmp() is also implemented > without READ_ONCE(), maybe we should change that...) > > > Second: You are assuming that if one side of the strcmp() is at most > four bytes long (including null terminator), then strcmp() also won't > access more than 4 bytes of the other string, even if that string does > not have a null terminator at index 4. I don't think that's part of > the normal strcmp() API contract. Ok, I'm convinced, all fair points. I'll switch to memcmp(), there is no downside to that anyways.