Re: [PATCH v7 07/23] firmware: arm_scmi: Add support to parse SHMTIs areas

Hi Cristian,

I found two UUID database issues that appear to be blockers.

	if (ti->info.num_uuids + SCMI_UUID_DB_THRESH >= ti->uuids_len) {
		uuid_t **uuids, **old_uuids;

		uuids = kcalloc(ti->uuids_len * 2, sizeof(*uuids),
				GFP_KERNEL);
		if (!uuids)
			return -ENOMEM;

		/* Copy/move old allocated UUIDs */
		for (int i = 0; i < ti->info.num_uuids; i++)
			uuid_copy(uuids[i], ti->info.uuids[i]);

uuids is a newly allocated and zeroed array of uuid_t pointers, so
uuids[i] is NULL here. uuid_copy() therefore copies into a NULL
destination when the database grows with existing entries.

The database stores pointers to UUIDs owned by telemetry_uuid objects, so
should this instead copy the pointers themselves, for example:

	uuids[i] = ti->info.uuids[i];

or use an appropriately sized memcpy() of the pointer array?

	ti->uuids_len = ti->num_shmti * 2;
	ti->info.uuids = kcalloc(ti->uuids_len,
				 sizeof(*ti->info.uuids),
				 GFP_KERNEL);

A valid implementation can have zero SHMTIs while exposing fast-channel or
notification-only DEs. In that case uuids_len is zero.

Primary UUID creation then enters the resize path, doubles zero to zero,
and eventually writes the primary UUID pointer through a zero-size
allocation.

Please give the UUID database a nonzero minimum initial capacity and use
checked growth so that zero cannot remain zero.

static void scmi_telemetry_line_put(struct telemetry_line *line,
				    void *blob)
{
	if (refcount_dec_and_test(&line->users)) {
		xa_erase(line->xa_lines,
			 (unsigned long)line->payld);
		kfree(blob);
	}
}

Lookups and refcount increments are serialized using lines_mtx, but this
final decrement, XArray erase and free are not performed under the same
lock.

A concurrent get-or-create operation can load the entry while another
thread decrements the refcount to zero and frees it. Please serialize the
final put with lookup/creation, or use a lifetime scheme such as
refcount_inc_not_zero() with appropriate XArray/RCU protection.

Thanks,
Fayçal