On Wed, Jul 10, 2024 at 1:45 PM Andi Kleen <ak@xxxxxxxxxxxxxxx> 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
>
