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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20250416143712.4e4666f4@endymion>
Date: Wed, 16 Apr 2025 14:37:12 +0200
From: Jean Delvare <jdelvare@...e.de>
To: Andrew Jeffery <andrew@...econstruct.com.au>
Cc: Joel Stanley <joel@....id.au>, Henry Martin <bsdhenrymartin@...il.com>,
 Patrick Rudolph <patrick.rudolph@...ements.com>, Andrew Geissler
 <geissonator@...oo.com>, Ninad Palsule <ninad@...ux.ibm.com>, Patrick
 Venture <venture@...gle.com>, Robert Lippert <roblip@...il.com>,
 linux-kernel@...r.kernel.org
Subject: Re: [PATCH 4/7] soc: aspeed: lpc-snoop: Constrain parameters in
 channel paths

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@...econstruct.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@...e.de>

-- 
Jean Delvare
SUSE L3 Support