Re: [PATCH v2] ptp: ocp: Limit SMA/signal/freq counts in show/store functions

[Jakub Kicinski <kuba@kernel.org>]

On Thu,  8 May 2025 10:19:01 +0300 Sagi Maimon wrote:
> The sysfs show/store operations could access uninitialized elements in
> the freq_in[], signal_out[], and sma[] arrays, leading to NULL pointer
> dereferences. This patch introduces u8 fields (nr_freq_in, nr_signal_out,
> nr_sma) to track the actual number of initialized elements, capping the
> maximum at 4 for each array. The affected show/store functions are updated to

This line is too long. I think the recommended limit for commit message
is / was 72 or 74 chars.

> respect these limits, preventing out-of-bounds access and ensuring safe
> array handling.

What do you mean by out-of-bounds access here. Is there any access with
index > 4 possible? Or just with index > 1 for Adva?

We need more precise information about the problem to decide if this is
a fix or an improvement 

> +	bp->sma_nr  = 4;

nit: double space in all the sma_nr assignments

>  
>  	ptp_ocp_fb_set_version(bp);
>  
> @@ -2862,6 +2870,9 @@ ptp_ocp_art_board_init(struct ptp_ocp *bp, struct ocp_resource *r)
>  	bp->fw_version = ioread32(&bp->reg->version);
>  	bp->fw_tag = 2;
>  	bp->sma_op = &ocp_art_sma_op;
> +	bp->signals_nr = 4;
> +	bp->freq_in_nr = 4;
> +	bp->sma_nr  = 4;
>  
>  	/* Enable MAC serial port during initialisation */
>  	iowrite32(1, &bp->board_config->mro50_serial_activate);
> @@ -2888,6 +2899,9 @@ ptp_ocp_adva_board_init(struct ptp_ocp *bp, struct ocp_resource *r)
>  	bp->flash_start = 0xA00000;
>  	bp->eeprom_map = fb_eeprom_map;
>  	bp->sma_op = &ocp_adva_sma_op;
> +	bp->signals_nr = 2;
> +	bp->freq_in_nr = 2;
> +	bp->sma_nr  = 2;
>  
>  	version = ioread32(&bp->image->version);
>  	/* if lower 16 bits are empty, this is the fw loader. */
> @@ -3002,6 +3016,9 @@ ptp_ocp_sma_show(struct ptp_ocp *bp, int sma_nr, char *buf,
>  	const struct ocp_selector * const *tbl;
>  	u32 val;
>  
> +	if (sma_nr > bp->sma_nr)
> +		return 0;

Why are you returning 0 and not an error?

As a matter of fact why register the sysfs files for things which don't
exists?
-- 
pw-bot: cr