Re: Fwd: iosm: detected field-spanning write for XMM7360

Kees Cook
2023-06-20T19:32:41Z

[regressions list to Bcc]

On Tue, Jun 20, 2023 at 11:12:40AM +0200, Linux regression tracking (Thorsten Leemhuis) wrote:
> On 20.06.23 10:44, Bagas Sanjaya wrote:
> > I notice a regression report on Bugzilla [1]. Quoting from it:
> [...]
> >> [Sa Jun 17 20:10:09 2023] memcpy: detected field-spanning write (size 16) of single field "&adth->dg" at drivers/net/wwan/iosm/iosm_ipc_mux_codec.c:852 (size 8)
> [...]
> > [1]: https://bugzilla.kernel.org/show_bug.cgi?id=217569

This looks like a legitimate bug.

Here are the structures used by drivers/net/wwan/iosm/iosm_ipc_mux_codec.c
leading up to the memcpy() triggering this warning at line 852, with
commentary from me added:

struct mux_adb {
struct sk_buff *dest_skb;
u8 *buf; // <- unbounded array
...
};

/**
* struct mux_adth - Structure of the Aggregated Datagram Table Header.
...
* @dg: datagramm table with variable length // variable length you say? :)
*/
struct mux_adth {
...
struct mux_adth_dg dg; // <- destination of memcpy
};

struct mux_adth_dg {
__le32 datagram_index;
__le16 datagram_length;
u8 service_class;
u8 reserved;
}; // 8 byte structure

static void ipc_mux_ul_encode_adth(struct iosm_mux *ipc_mux,
struct mux_adb *ul_adb, int *out_offset)
{
...
struct mux_adth *adth;
...
// Assignment of fixed-sized structure on an
// unbounded string (i.e. minimum size
// constraint is now defined by structure
// layout, which is a good thing.)
adth = (struct mux_adth *)&ul_adb->buf[offset];
...
// This appears to be preparing for having
// _multiple_ "dg" structs at the end of
// struct mux_adth_dg, not just 1. (And, if so,
// should be using the struct_size(), or really,
// given the later subtraction, the flex_size(),
// helper.)
adth_dg_size = offsetof(struct mux_adth, dg) +
ul_adb->dg_count[i] * sizeof(*dg);
...
adth_dg_size -= offsetof(struct mux_adth, dg);
memcpy(&adth->dg, ul_adb->dg[i], adth_dg_size);

&adth->dg is 8 bytes, so the warning message is correct. However, it
seems like "ul_adb->dg_count[i]" contains a _count_ of "dg" structures
to copy, and, in the reported case, is "2".

The fix appears to be adjusting struct mux_adth with:

- struct mux_adth_dg dg;
+ struct mux_adth_dg dg[];

But, since this is an implicit "1-element array to flexible array"
conversion[1], we need to double-check "sizeof" uses.

I only see 3 cases of sizeof(struct mux_adth). This one removes the
"extra" "dg" element for bounds checking:

if (le16_to_cpu(adth->table_length) < (sizeof(struct mux_adth) -
sizeof(struct mux_adth_dg)))

This one adds it back in after subtracting 1 too many:

nr_of_dg = (le16_to_cpu(adth->table_length) -
sizeof(struct mux_adth) +
sizeof(struct mux_adth_dg)) /
sizeof(struct mux_adth_dg);

As does this one:

nr_of_dg = (le16_to_cpu(adth->table_length) -
sizeof(struct mux_adth) +
sizeof(struct mux_adth_dg)) /
sizeof(struct mux_adth_dg);

So they can be simplified to avoid the extra math.

I'll send a patch...

-Kees

[1] https://github.com/KSPP/linux/issues/79

--
Kees Cook