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

[Kees Cook <keescook@xxxxxxxxxxxx>]

[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