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