Re: Re: [PATCH v2] cxl/mbox: bound the Get Supported Logs entry count by the payload

[黄高彬]

Hi Alison,

Thanks for the review, and sorry for the slow turn-around.

> Are we intentionally bounding the enumeration and continuing with a partial
> response, or should we be validating the device supplied count and rejecting
> an inconsistent response, as get_supported_features does?

Validating and rejecting, and Richard's review on this thread moved the check
to where it belongs.  There is no reason to salvage the entries that fit:
CXL r4.0 Table 8-249 defines the count as the number of entries returned in
this payload, not a running total, so a count the payload cannot hold is a
malformed response rather than a partial list.  v3 therefore validates inside
cxl_get_gsl(), which issues the command, owns the buffer and can return the
error itself:

	mbox_cmd = (struct cxl_mbox_cmd) {
		.opcode = CXL_MBOX_OP_GET_SUPPORTED_LOGS,
		.size_out = cxl_mbox->payload_size,
		.payload_out = ret,
		/* At least the header must be valid */
		.min_out = struct_size(ret, entry, 0),
	};
	rc = cxl_internal_send_cmd(cxl_mbox, &mbox_cmd);
	if (rc < 0) {
		kvfree(ret);
		return ERR_PTR(rc);
	}

	max_entries = (mbox_cmd.size_out - struct_size(ret, entry, 0)) /
		      sizeof(ret->entry[0]);
	if (le16_to_cpu(ret->entries) > max_entries) {
		dev_err(mds->cxlds.dev,
			"GSL: device claimed %u entries but the payload holds %zu\n",
			le16_to_cpu(ret->entries), max_entries);
		kvfree(ret);
		return ERR_PTR(-EIO);
	}

cxl_enumerate_cmds() is unchanged now, and no caller can see an entry without
a count it can trust.  Raising min_out to the header also lets
cxl_internal_send_cmd() reject a response too short to hold the count, so the
manual length check -- and the subtraction it had to guard -- are gone.

That also makes the last paragraph of v2's commit message true.  v2 claimed to
be the cross-check get_supported_features() performs, but it bounded the loop
and carried on, which is not what that function does.  Thank you for catching
the contradiction.

One consequence to be explicit about: cxl_enumerate_cmds() is called from
cxl_pci_probe(), so a device whose count does not fit the payload now fails to
probe instead of coming up with the entries that fit.  That is the loud failure
you both asked for.

> I don't think the comment is needed. The check and error message make the
> requirement clear. As written, it is good self-documenting code :)

The comment is gone.  So is the one on the *len assignment -- that assignment
is gone too, since cxl_get_gsl() now does the validation itself.

> Nit: CXL subsystem begins subjects w uppercase

Capitalized: "cxl/mbox: Bound the Get Supported Logs entry count by the
payload".

> Also, I don't think you can claim no existing hardware is affected.

Dropped -- I cannot rule that out, so the commit message only says what the
response may contain and what the driver does about it.

> Note that once this patch is applied, the text below the scissors lines
> disappears and referencing 1/2 loses meaning.

The changelog now describes v2 -> v3 only, and nothing else refers to the
dropped patch.

> 	The Get Supported Logs response includes a device supplied entry count.
> 	...
> 	Reproduced with QEMU modified to return an inconsistent entry count.
> 	KASAN reported an out-of-bounds read in cxl_enumerate_cmds().

Adopted.  The 102/103 sweep and the boundary arithmetic are gone; the repro is
one sentence.

> Seconded. Error out as early as it is convenient to do validation.
> Here that is as Alison says before the loop starts.

Done, as above.

sashiko also reported the leak on v2, and Richard pointed it out
independently; the early return it concerned is gone with the restructure.

v3:
https://lore.kernel.org/all/20260929060610.3549718-1-huanggaobin23@xxxxxxxxxx/

Gaobin