Re: [PATCH net-next v5 4/7] ptp: ocp: Track EEPROM fields independently

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Kernel stack memory disclosure to userspace due to uninitialized buffer used with short reads.
- [Medium] The EEPROM discovery loop aborts on the first error, contradicting the commit's intent to track fields independently.
- [Medium] Internal `-EPROBE_DEFER` error code is leaked to userspace via the `serialnum_show` sysfs attribute.
--

--- Patch [4]: [PATCH net-next v5 4/7] ptp: ocp: Track EEPROM fields independently ---
commit c7acafdfcb181ccb357d00ccd6cff4bb48838592
Author: Ahmad Byagowi <[email protected]>

ptp: ocp: Track EEPROM fields independently

This patch updates the EEPROM discovery logic to serialize reads and
track the availability of the board ID and serial number independently.
It aims to ensure that failure to read one field does not prevent the
discovery or publication of the other field.

> diff --git a/drivers/ptp/ptp_ocp.c b/drivers/ptp/ptp_ocp.c
> index 3d26ec1f7b9ec..31a4c9dfe391c 100644
> --- a/drivers/ptp/ptp_ocp.c
> +++ b/drivers/ptp/ptp_ocp.c
[ ... ]
> @@ -1969,21 +1977,50 @@ ptp_ocp_nvmem_device_put(struct nvmem_device **nvmemp)
>  	*nvmemp = NULL;
>  }
>  
> -static void
> -ptp_ocp_read_eeprom(struct ptp_ocp *bp)
> +static int
> +ptp_ocp_read_eeprom(struct ptp_ocp *bp, unsigned int fields)
>  {
>  	const struct ptp_ocp_eeprom_map *map;
>  	struct nvmem_device *nvmem;
> +	u8 data[OCP_BOARD_ID_LEN];
> +	const char *field_name = "data";
>  	const void *tag;
> -	int ret;
> -
> -	if (!bp->i2c_ctrl)
> -		return;
> +	unsigned int field;
> +	int ret = 0;
>  
>  	tag = NULL;
>  	nvmem = NULL;
[ ... ]
>  	for (map = bp->eeprom_map; map->len; map++) {
[ ... ]
> +		if (map->tag != tag) {
>  			tag = map->tag;
>  			ptp_ocp_nvmem_device_put(&nvmem);
> +		}
> +		if (!nvmem) {
>  			nvmem = ptp_ocp_nvmem_device_get(bp, tag);
>  			if (IS_ERR(nvmem)) {
>  				ret = PTR_ERR(nvmem);
> -				goto fail;
> +				goto out;

[Severity: Medium]
Since the commit message states the intent is to avoid hiding an otherwise
valid board ID when the serial EEPROM block is missing, does aborting the
loop here on the first error contradict that?

If nvmem_device_get() fails for the first entry in bp->eeprom_map, this
goto out will skip reading any subsequent entries.

>  			}
>  		}
> -		ret = nvmem_device_read(nvmem, map->off, map->len,
> -					BP_MAP_ENTRY_ADDR(bp, map));
> -		if (ret != map->len)
> -			goto fail;
> +		ret = nvmem_device_read(nvmem, map->off, map->len, data);
> +		if (ret < 0)
> +			goto out;

[Severity: Medium]
Similarly, if nvmem_device_read() fails for an entry, won't this goto out
also prevent subsequent fields from being discovered?

> +
> +		memcpy(BP_MAP_ENTRY_ADDR(bp, map), data, map->len);

[Severity: High]
Does this allow uninitialized stack memory to be copied to the persistent
device structure?

The data buffer is allocated uninitialized on the stack. If nvmem_device_read()
returns a short read (where 0 <= ret < map->len), the error check above
(ret < 0) will pass. The memcpy will then copy the entire map->len bytes,
including uninitialized kernel stack data, which could later be disclosed to
userspace via sysfs attributes like serialnum_show or the devlink interface.

> +		if (field == OCP_EEPROM_BOARD_ID) {
[ ... ]
> @@ -3757,9 +3805,17 @@ static ssize_t
>  serialnum_show(struct device *dev, struct device_attribute *attr, char *buf)
>  {
>  	struct ptp_ocp *bp = dev_get_drvdata(dev);
> +	int ret;
>  
> -	if (!bp->has_eeprom_data)
> -		ptp_ocp_read_eeprom(bp);
> +	/* Pairs with field publication in ptp_ocp_read_eeprom(). */
> +	if (!smp_load_acquire(&bp->has_serial)) {
> +		ret = ptp_ocp_read_eeprom(bp, OCP_EEPROM_SERIAL);
> +		if (ret)
> +			return ret;

[Severity: Medium]
Can this leak internal kernel error codes to userspace?

If nvmem_device_get() is not yet ready, ptp_ocp_read_eeprom() legitimately
returns -EPROBE_DEFER. Returning this directly from the sysfs show handler
will expose -EPROBE_DEFER (-517) to userspace applications, breaking the
POSIX ABI. Should this be translated to -ENODATA or -EAGAIN?

> +		/* Pairs with field publication in ptp_ocp_read_eeprom(). */
> +		if (!smp_load_acquire(&bp->has_serial))
> +			return -ENODATA;
> +	}
>  
>  	return sysfs_emit(buf, "%pM\n", bp->serial);
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=4