Re: [PATCH 4/7] soc: aspeed: lpc-snoop: Constrain parameters in channel paths

[Jean Delvare <jdelvare@suse.de>]

On Fri, 11 Apr 2025 10:38:34 +0930, Andrew Jeffery wrote:
> Ensure pointers and the channel index are valid before use.
> 
> Fixes: 9f4f9ae81d0a ("drivers/misc: add Aspeed LPC snoop driver")

Please don't abuse Fixes tags. Here you are hardening the code just in
case, but this isn't fixing any actual bug, as functions
aspeed_lpc_enable_snoop() and aspeed_lpc_disable_snoop() were never
called with an incorrect channel index.

> Signed-off-by: Andrew Jeffery <andrew@codeconstruct.com.au>
> ---
>  drivers/soc/aspeed/aspeed-lpc-snoop.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/drivers/soc/aspeed/aspeed-lpc-snoop.c b/drivers/soc/aspeed/aspeed-lpc-snoop.c
> index 28f034b8a3b7226efe20cbe30a7da0c2b49fbd96..6ab362aeb180c8ad356422d8257717f41a232b3c 100644
> --- a/drivers/soc/aspeed/aspeed-lpc-snoop.c
> +++ b/drivers/soc/aspeed/aspeed-lpc-snoop.c
> @@ -182,6 +182,7 @@ static int aspeed_lpc_snoop_config_irq(struct aspeed_lpc_snoop *lpc_snoop,
>  	return 0;
>  }
>  
> +__attribute__((nonnull))
>  static int aspeed_lpc_enable_snoop(struct aspeed_lpc_snoop *lpc_snoop,
>  				   struct device *dev,
>  				   int channel, u16 lpc_port)
> @@ -190,6 +191,8 @@ static int aspeed_lpc_enable_snoop(struct aspeed_lpc_snoop *lpc_snoop,
>  	u32 hicr5_en, snpwadr_mask, snpwadr_shift, hicrb_en;
>  	int rc = 0;
>  
> +	if (channel < 0 || channel >= ARRAY_SIZE(lpc_snoop->chan))
> +		return -EINVAL;
>  
>  	if (lpc_snoop->chan[channel].enabled)
>  		return -EBUSY;
> @@ -252,9 +255,13 @@ static int aspeed_lpc_enable_snoop(struct aspeed_lpc_snoop *lpc_snoop,
>  	return rc;
>  }
>  
> +__attribute__((nonnull))
>  static void aspeed_lpc_disable_snoop(struct aspeed_lpc_snoop *lpc_snoop,
>  				     int channel)
>  {
> +	if (channel < 0 || channel >= ARRAY_SIZE(lpc_snoop->chan))
> +		return;
> +
>  	if (!lpc_snoop->chan[channel].enabled)
>  		return;
>  
> 

TBH I'm not sure if this has much value, as any error in the channel
index (or passing NULL pointers for lpc_snoop or dev) would likely be
caught very early during driver development or update. And silently
returning early is not going to fix the problem if this ever happens.

But well, I'm not much into defensive programming anyway, so maybe this
is just me. As I'm not maintaining this driver, this ain't my decision.

Acked-by: Jean Delvare <jdelvare@suse.de>

-- 
Jean Delvare
SUSE L3 Support