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 D1250C3DA4A for ; Wed, 14 Aug 2024 16:14:42 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 686BD6B0088; Wed, 14 Aug 2024 12:14:42 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 636486B009A; Wed, 14 Aug 2024 12:14:42 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 4FE256B009B; Wed, 14 Aug 2024 12:14:42 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0015.hostedemail.com [216.40.44.15]) by kanga.kvack.org (Postfix) with ESMTP id 3007E6B0088 for ; Wed, 14 Aug 2024 12:14:42 -0400 (EDT) Received: from smtpin09.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay07.hostedemail.com (Postfix) with ESMTP id DB97D16107A for ; Wed, 14 Aug 2024 16:14:41 +0000 (UTC) X-FDA: 82451349162.09.CFF89C4 Received: from mail-ed1-f50.google.com (mail-ed1-f50.google.com [209.85.208.50]) by imf14.hostedemail.com (Postfix) with ESMTP id DAD2C100008 for ; Wed, 14 Aug 2024 16:14:39 +0000 (UTC) Authentication-Results: imf14.hostedemail.com; dkim=pass header.d=google.com header.s=20230601 header.b=1CJa2Ive; dmarc=pass (policy=reject) header.from=google.com; spf=pass (imf14.hostedemail.com: domain of jannh@google.com designates 209.85.208.50 as permitted sender) smtp.mailfrom=jannh@google.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1723652021; a=rsa-sha256; cv=none; b=E4P49LfTmlZi5oHGdxsxdrkzhghgujSVxO9edlsS+XNHtgmzLN82DkExLU8CpSnL8YXxwL JzLxaVfSrHTgNBagt8wtEB7B+a47DHhDpjmvkaYFbIF0PJ3roskn7JzlkhI6isXkVs+jQp VHYJXdDTPIkcSRruNjSFhSUb6kMJF2o= ARC-Authentication-Results: i=1; imf14.hostedemail.com; dkim=pass header.d=google.com header.s=20230601 header.b=1CJa2Ive; dmarc=pass (policy=reject) header.from=google.com; spf=pass (imf14.hostedemail.com: domain of jannh@google.com designates 209.85.208.50 as permitted sender) smtp.mailfrom=jannh@google.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1723652021; 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=6bQ9j9ErtrrpZhSLzeUe548UlFsO7KLb0/TyLtNcWJw=; b=gRMqZghtOPvvcbsPcz/iojuOjJ5b8u86oM6HuHucS9OVo2jvJymPIq773QKDKiIICdoD9v ilebztubm7GUBx2Hb6EO7ke1YTrVmsOkIBBHBDhi1VYtPQxRjeZE1/S+cP5JgNOfdeNG1N SWVAKCzobfzY1fCRzuktiL79f8jRb+w= Received: by mail-ed1-f50.google.com with SMTP id 4fb4d7f45d1cf-5b9fe5ea355so8522a12.0 for ; Wed, 14 Aug 2024 09:14:39 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20230601; t=1723652078; x=1724256878; 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=6bQ9j9ErtrrpZhSLzeUe548UlFsO7KLb0/TyLtNcWJw=; b=1CJa2Ive4yDP1+UlwjE1Yhmd9WopkiHhfW2r1DECccXkV0h4pt6aJBcxFJD4foPK7t gMKvXrwZGNj02DM/1wuKfdZfQDYFBiYxIAiNecBITUfqupXGtBZ4dJDkIxA6oU/8UWc8 y1C5JsN04yH7el8TroLY+Go1PBBcuQtXwVe9i09BEWJhNS+It6gNMLZYc0lnDLQzu/Wl BgusrHgsQKLToZxYKGYQOzZ41o9W0dGd584NcNzevNx5qLr2sC8B6B+ABev6zkDy3eLw kPFjSXIioDcDaEIeqSabwzPoLQD7ykJC8PTQgH1dWYXajZ47g6li0cWYw2yk9m8i9jbf XUcA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1723652078; x=1724256878; 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=6bQ9j9ErtrrpZhSLzeUe548UlFsO7KLb0/TyLtNcWJw=; b=bbA/CHvCPfsisUoaT7m8oO+ilsLJrT+0n8n+30zpYM8R4plB1CI/xx3mqz0tZVlJr0 9TefJ5wTbgZYpZEsxXLAOFQCgjoKoqpCFgPEhcpvwPph4+P6HYldyZTk6ZiA/PivKDTj pxUgok5DGkFgeSUEv9DBayxJBQbUs+gqVoNwa/IgzxGjo7puvTWH1X0Ix+S3oXWGDMng i44+Or9CC2AxG1ZCyIflHWCSKTV48O5mBjrj36jcHW5zDocSSuxHObQCH33bYeh+20O4 MQ2WFKcWnqfdQxBDcqf9T7ty7BIDC93co0zooCjHbk9Q62vs+lNFKpiPWsc2REnGKI4k FV+w== X-Forwarded-Encrypted: i=1; AJvYcCWXBhjsW4Zp+RnL99j4Z6DrIvlyUGXVh3gYBk3FGuQQ0tILOo1BzVe+Ohb69KX2L7S+e1wfK+jrHQ==@kvack.org X-Gm-Message-State: AOJu0YwXRjmJLSfJmUmVC3DOO93f6FdordYYJx0gr0SNE1jedJukcUdo 7/ykwguLmOz1zdAYZtLJa793cYOwnp4w7l4MXSXk/vPMeJ8cHQCb02MmjztGxSumElsF/PGL94N ZSVsgpswoAOYka2w5upe8gaWxGUXAbJYCM9r5 X-Google-Smtp-Source: AGHT+IEDxZPD3vJxwWTsDeOPtGLySgwgkgr6dRkg5BY1PBUpXws7SIHVDHMiXTM6viwfIH4s0+WA4qXy/HT+Mlhdu7E= X-Received: by 2002:a05:6402:50cf:b0:58b:93:b623 with SMTP id 4fb4d7f45d1cf-5bea55635d8mr87569a12.5.1723652077454; Wed, 14 Aug 2024 09:14:37 -0700 (PDT) MIME-Version: 1.0 References: <20240813002932.3373935-1-andrii@kernel.org> <20240813002932.3373935-2-andrii@kernel.org> In-Reply-To: From: Jann Horn Date: Wed, 14 Aug 2024 18:13:59 +0200 Message-ID: Subject: Re: [PATCH v5 bpf-next 01/10] lib/buildid: harden build ID parsing logic To: Andrii Nakryiko 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: DAD2C100008 X-Stat-Signature: tz4wknqwpqr9oywa814coobzn9wox38f X-Rspam-User: X-HE-Tag: 1723652079-291325 X-HE-Meta: U2FsdGVkX1+9VH1lpp5eLF90KZRCnj65UVFXtJMaTLUWcjlCP9YIP2nv/OYtwl+nupvSNi9xMak+GkDYLUfmRrlncF7pTuHPzATWt+ERakDaNUpQr2MF5q9ZM58X/CD1nzopQkPIaisjMLeoF3VRqanhMkh0IXKSsJa7NOFnBqq0BTB/dzY0ovzHOZgipokiF6QydO9EFsuZmg6X8GTGV1MxsAlN4NQRGQA+WXiyYVWUdJXsfkCqU2FiWSwRNk86ye0MefIbR3fa6yAmiT5dDlDWmHYFA3sokTS4mVUCfJGgfnyZvE77PHjYDNIagC9vOrK7YQODN+ulOPHW7QsW/6Po32ckmrTg2FrgNOXizrUGDrPmK2ug4y9U2oV+Kx1Y0hm+wPTT0rBJbwRldjdHAjw8D3Ou86FcR5r3oryWhgmBNkJYFg0xTl9Mtpb0bAfKWjbUc5u4IpNO4FzCxkOeGdT/RpZthocryIOiZLaBWFzoRa30TG1fmY/zX3F6diHOrFMpIGWsAudFjHWvsKU1PTybXZfqMb4Q1IGSdo6dxjbcrrZ8t9x6PW7sVkljVKivof0AuzVP+fZSCof97wY+jHJd1F+/EP4N1ZEotWY9GS0Zl1tdrqOsvXqdtAwqc+EhPJRtZuAz5y27YGWTY21RCGF1YGQ/d4xHpUDOZuoYBEuW9Ef01ywN0rI6HoB2EMW3mfh4StUF4+KLq088VnuXi5GH0amfySbXIovWpoSSzuHw3jJ4JUt/DoxjnGxqcv7KH4EvhNPKNPGi+PoZ+tZG4OUc066Lk++wn01+kbNZy3ARHguSr8F6xpfGx0vVyqYbrhiNBU5ANH/39+xRlkwSfX0x3Njp7NPdQqr9oSZOUoQDJ7e4TLZyz2i/VUXzqr2TuGPvWouuYiPQvyefwIz0Y6fA516sGIX7lzQLrvT7Nb1JoioVoYH1FFgilJ/+4RdyfLuBR26TzlOfly3xcuA 1eypxYRc CZil4+pBTgVoUVS45Px8LX/I4lFA+dbxu6n/lxWKUHLpkYDoEH5zjuwts/WB+QTw/wxlhbQRsrdL31aF6+Bw0cdEVIph68c0JBrrzhxWaKosR3jS+6jjlS5vCmlFRO52N5HGumOJfXmoQNNj5kr5AGHkCUJnoZzKCtsCYteACn1gWiZN/pUkfY9cKEDVm+NVFrKJJDe9fUBCMOzIL39heKmcEn8lo/xyHxq9yBUGmodlnfVAm2vi2xQ88MlNBXaJeFaQDdzCxTQHwNul1XoZDQIOpNCpgw5NHf8Fb8H5xSzhZrrr5cKMyqspW4ogBquHI+YGI1QAsheYnLlW68DJKRKIlEMepavOIh0K0 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 1:21=E2=80=AFAM Andrii Nakryiko wrote: > On Tue, Aug 13, 2024 at 1:59=E2=80=AFPM Jann Horn wrot= e: > > > > 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 = ELF > > > note is within a page bounds, so move the overflow check up and add a= n > > > 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, maki= ng > > > 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 *buil= d_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= _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 the = 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 change. 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 allowed= . (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.