Re: [PATCH] greybus: audio: bound the topology section sizes against the fetched size

[Bryam Vargas]

Hi Greg, and thanks Dan,

>> Are you sure these checks will not overflow?
> Yep.  The cast to u64 ensures that.

Right, and to close the other side of the comparison too: `size` is a u16 and
the function already does `if (size < sizeof(*topo)) return -ENODATA;` above
this point, so `size - sizeof(*topo)` cannot underflow either. The left side is
the (u64) sum of four u32s (max ~2^34), so neither side wraps. The form
`sizeof(*topo) + sum > size` is exactly equivalent if it reads more clearly.

> But we trust the hardware to send us proper data, right?  If we don't trust
> modules, then there are lots of other places stuff like this needs to be
> fixed, how many data paths did you audit?

I audited the four size_* fields that gbaudio_tplg_parse_data() turns into
section offsets -- those are the only module-supplied values that feed directly
into unchecked pointer arithmetic (control/widget/route_offset are dai_offset
plus those le32s, then dereferenced as structs). I am not claiming a broader
greybus or topology-parser audit; that is welcome but separate.

It is less "modules are malicious" than "a malformed or buggy module response
should not walk the parser off a slab object" -- the same
untrusted-length-to-offset shape already hardened for USB/HID/BT descriptors.
If you would rather treat module data as trusted and drop the stable tag, that
is your call; I would keep the bound regardless, since it is one branch and the
offsets are otherwise completely unchecked.

> How did you find/fix this?  You need to list what tools helped you...

I do not have real greybus audio hardware, so I simulated the module side and
drove the negative case directly: a topology whose fetched `size` is small but
whose size_* fields are large -- exactly the invariant this patch enforces.
With that I reproduced the read two ways:

  - in-kernel under KASAN (7.1.0-rc5): slab-out-of-bounds 4 bytes past a
    kmalloc-64 object; the patched arm (-EINVAL) and an in-bounds arm are clean;
  - a userspace AddressSanitizer model of the process_header() offset walk,
    both -m32 and -m64.

Tools: a static read of the audio_gb.c -> audio_topology.c data flow, a litmus
greybus module under KASAN in a VM, and the userspace ASan harness. The
verifiable artifact is the KASAN splat (trimmed under the --- in the original
posting; full log on request).

Thanks,
Bryam