From: Andrii Nakryiko <andrii.nakryiko@gmail.com>
To: Andi Kleen <ak@linux.intel.com>
Cc: Andrii Nakryiko <andrii@kernel.org>,
bpf@vger.kernel.org, linux-mm@kvack.org,
akpm@linux-foundation.org, adobriyan@gmail.com,
shakeel.butt@linux.dev, hannes@cmpxchg.org, osandov@osandov.com
Subject: Re: [PATCH bpf-next 07/10] lib/buildid: harden build ID parsing logic some more
Date: Wed, 10 Jul 2024 13:53:23 -0700 [thread overview]
Message-ID: <CAEf4BzbNttJUw2bntQ+GWLgJDxtwmC_NNBQ4Y7j2JuN6FuYQ9w@mail.gmail.com> (raw)
In-Reply-To: <Zo7y0O8wm0_xZ0li@tassilo>
On Wed, Jul 10, 2024 at 1:45 PM Andi Kleen <ak@linux.intel.com> wrote:
>
> On Tue, Jul 09, 2024 at 01:42:42PM -0700, Andrii Nakryiko wrote:
> > Harden build ID parsing logic some more, adding explicit READ_ONCE()
> > when fetching values that we then use to check correctness and various
> > note iteration invariants.
>
> Just sprinkling READ_ONCE all over doesn't necessarily fix the code.
> It is only needed for values that affect a loop or reference.
Agreed, besides `READ_ONCE(nhdr->n_type) == BUILD_ID` and
`READ_ONCE(phdr->p_type) == PT_NOTE`, which I added mostly just for
consistency, the rest should be indeed read once and then checked, no?
Do you see any other unnecessary READ_ONCE()s in this patch?
>
> You have to fix stuff like this
>
> static inline int parse_build_id(const void *page_addr,
> unsigned char *build_id,
> __u32 *size,
> const void *note_start,
> Elf32_Word note_size)
> {
> /* check for overflow */
> if (note_start < page_addr || note_start + note_size < note_start)
> ^^^^^^^^^^^^^^^^^^^^^^
this has been switched to u64-based offsets in patch #1, did you take
a look at it?
> return -EINVAL;
>
>
> which is C undefined (at least without -fwrapv-pointer) and can easily
> be miscompiled if it isn't already.
>
> I suspect the code will need more work, especially since you're
> unwilling to consider any defense in depth measures.
>
Can you be a bit more specific about the remaining issues? I'm happy
to fix whatever can and should be fixed (after the changes I already
did in this patch set).
If by "defense in depth" you mean allowing this functionality only for
executable VMAs, then yes, I refuse to do that, as I already
explained.
> -Andi
>
next prev parent reply other threads:[~2024-07-10 20:53 UTC|newest]
Thread overview: 14+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-07-09 20:42 [PATCH bpf-next 00/10] Harden and extend ELF build ID parsing logic Andrii Nakryiko
2024-07-09 20:42 ` [PATCH bpf-next 01/10] lib/buildid: add single page-based file reader abstraction Andrii Nakryiko
2024-07-09 20:42 ` [PATCH bpf-next 02/10] lib/buildid: take into account e_phoff when fetching program headers Andrii Nakryiko
2024-07-09 20:42 ` [PATCH bpf-next 03/10] lib/buildid: remove single-page limit for PHDR search Andrii Nakryiko
2024-07-09 20:42 ` [PATCH bpf-next 04/10] lib/buildid: rename build_id_parse() into build_id_parse_nofault() Andrii Nakryiko
2024-07-09 20:42 ` [PATCH bpf-next 05/10] lib/buildid: implement sleepable build_id_parse() API Andrii Nakryiko
2024-07-09 20:42 ` [PATCH bpf-next 06/10] lib/buildid: don't limit .note.gnu.build-id to the first page in ELF Andrii Nakryiko
2024-07-09 20:42 ` [PATCH bpf-next 07/10] lib/buildid: harden build ID parsing logic some more Andrii Nakryiko
2024-07-10 20:45 ` Andi Kleen
2024-07-10 20:53 ` Andrii Nakryiko [this message]
2024-07-09 20:42 ` [PATCH bpf-next 08/10] bpf: decouple stack_map_get_build_id_offset() from perf_callchain_entry Andrii Nakryiko
2024-07-09 20:42 ` [PATCH bpf-next 09/10] bpf: wire up sleepable bpf_get_stack() and bpf_get_task_stack() helpers Andrii Nakryiko
2024-07-09 20:42 ` [PATCH bpf-next 10/10] selftests/bpf: add build ID tests Andrii Nakryiko
2024-07-10 17:55 ` Andrii Nakryiko
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CAEf4BzbNttJUw2bntQ+GWLgJDxtwmC_NNBQ4Y7j2JuN6FuYQ9w@mail.gmail.com \
--to=andrii.nakryiko@gmail.com \
--cc=adobriyan@gmail.com \
--cc=ak@linux.intel.com \
--cc=akpm@linux-foundation.org \
--cc=andrii@kernel.org \
--cc=bpf@vger.kernel.org \
--cc=hannes@cmpxchg.org \
--cc=linux-mm@kvack.org \
--cc=osandov@osandov.com \
--cc=shakeel.butt@linux.dev \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox