Re: [PATCH v6 09/17] nvmem: microchip-otpc: nvmem: microchip-otpc: add tag-based packet lookup

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [High] TAG-based DT cells will be rejected by the nvmem core due to out-of-bounds offset values.

Pre-existing issues:
- [High] Concurrent accesses to nvmem_reg_read will corrupt OTP controller registers.
- [High] Out-of-bounds MMIO read during probe due to `size` vs `word_pos` miscalculation.
--

--- Patch [9]: [PATCH v6 09/17] nvmem: microchip-otpc: nvmem: microchip-otpc: add tag-based packet lookup ---
commit ffccf750925ce25af706c5e14106e91f61b012bc
Author: Varshini Rajendran <varshini.rajendran@microchip.com>

nvmem: microchip-otpc: nvmem: microchip-otpc: add tag-based packet lookup

Add support for accessing OTP packets by their tag which is a FourCC
while preserving backward compatibility with the existing ID-based lookup.
During probe, packet meta data including the tag is read and cached.

> diff --git a/drivers/nvmem/microchip-otpc.c b/drivers/nvmem/microchip-otpc.c
> index df979e8549fdb..a17f82afdc37d 100644
> --- a/drivers/nvmem/microchip-otpc.c
> +++ b/drivers/nvmem/microchip-otpc.c
> @@ -70,6 +74,52 @@ static struct mchp_otpc_packet *mchp_otpc_id_to_packet(struct mchp_otpc *otpc,

[ ... ]

> +static struct mchp_otpc_packet *mchp_otpc_resolve_packet(struct mchp_otpc *otpc, u32 off)
> +{
> +	/*
> +	 * Legacy id based packet access: offset = id * 4
> +	 * Inside the driver we use continuous unsigned integer numbers
> +	 * for packet id, thus divide off by 4 before passing it to
> +	 * mchp_otpc_id_to_packet().
> +	 */
> +	u32 remainder = off % 4;
> +	u32 id = off / 4;
> +
> +	if (!remainder && id < otpc->npackets)
> +		return mchp_otpc_id_to_packet(otpc, id);
> +
> +	/*
> +	 * TAG-based packet access: offset is a 4-byte ASCII tag (FourCC)
> +	 */
> +	return mchp_otpc_tag_to_packet(otpc, off);

[Severity: High]
Will TAG-based DT cells be rejected by the nvmem core due to out-of-bounds
offset values?

The nvmem core strictly validates DT cells in nvmem_add_cells_from_dt() using:
if (cell->offset >= nvmem->size || cell->bytes > nvmem->size - cell->offset)

Since the TAG value (e.g., 0x41435354) massively exceeds the physical size of
the OTP memory, will the core unconditionally reject the cell as out-of-bounds
and fail to register it?

> +}
> +

[ ... ]

> @@ -154,12 +225,11 @@ static int mchp_otpc_read(void *priv, unsigned int off, void *val,
>  	int ret, payload_size;
>  
>  	/*
> -	 * We reach this point with off being multiple of stride = 4 to
> -	 * be able to cross the subsystem. Inside the driver we use continuous
> -	 * unsigned integer numbers for packet id, thus divide off by 4
> -	 * before passing it to mchp_otpc_id_to_packet().
> +	 * From this point the offset has to be translated into the actual
> +	 * packet. For this we traverse the table of contents stored in a list
> +	 * "packet" based on the access type - packet id or tag.
>  	 */
> -	packet = mchp_otpc_id_to_packet(otpc, off / 4);
> +	packet = mchp_otpc_resolve_packet(otpc, off);
>  	if (!packet)
>  		return -EINVAL;
>  	offset = packet->offset;

[Severity: High]
This is a pre-existing issue, but does mchp_otpc_read() lack synchronization
when communicating with the hardware?

The nvmem core's sysfs binary file interface does not serialize reg_read
accesses across concurrent readers. When mchp_otpc_read() calls
mchp_otpc_prepare_read(), it performs a multi-register stateful sequence
(MCHP_OTPC_MR write, MCHP_OTPC_CR write, wait on MCHP_OTPC_SR) without holding
any lock.

Could concurrent reads interleave register writes and cause the hardware to
read from incorrect addresses or stall indefinitely?

[ ... ]

> @@ -215,6 +300,20 @@ static int mchp_otpc_init_packets_list(struct mchp_otpc *otpc, u32 *size)
>  
>  		packet->id = id++;
>  		packet->offset = word_pos;
> +		type = FIELD_GET(MCHP_OTPC_HR_PACKET, word);
> +
> +		if (type == MCHP_OTPC_HR_PACKET_REGULAR) {
> +			ret = mchp_otpc_read_packet_tag(otpc, packet->offset,
> +							&packet->tag);
> +			if (ret)
> +				return ret;
> +
> +			if (!mchp_otpc_is_valid_fourcc(packet->tag))
> +				packet->tag = 0;
> +		} else {
> +			packet->tag = 0;
> +		}
> +
>  		INIT_LIST_HEAD(&packet->list);
>  		list_add_tail(&packet->list, &otpc->packets);
>  

[Severity: High]
This isn't a bug introduced by this patch, but does the loop in
mchp_otpc_init_packets_list() perform an out-of-bounds MMIO read?

The loop condition is while (*size < MCHP_OTPC_SIZE). During each iteration,
*size advances by 4 * (payload_size + 1) bytes, but word_pos advances by
payload_size + 2 words (which is 4 * (payload_size + 2) bytes).

Because word_pos grows faster than *size, won't it exceed the physical
hardware bounds well before the loop terminates? If so, the subsequent call to
mchp_otpc_prepare_read(otpc, word_pos) could write this out-of-bounds offset
into the hardware address register.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260806074024.531259-1-varshini.rajendran@microchip.com?part=9