linux-kernel - Re: [PATCH v2 04/10] 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] [day] [month] [year] [list]

Message-ID: <f66b8b80d057a095dad3b41f224fb633565313ce.camel@codeconstruct.com.au>
Date: Tue, 08 Jul 2025 11:36:39 +0930
From: Andrew Jeffery <andrew@...econstruct.com.au>
To: Jean Delvare <jdelvare@...e.de>
Cc: linux-aspeed@...ts.ozlabs.org, 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-arm-kernel@...ts.infradead.org,
 linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 04/10] soc: aspeed: lpc-snoop: Constrain parameters
 in channel paths

Hi Jean,

On Fri, 2025-07-04 at 18:44 +0200, Jean Delvare wrote:
> On Mon, 16 Jun 2025 22:43:41 +0930, Andrew Jeffery wrote:
> > Ensure pointers and the channel index are valid before use.
> > 
> > Signed-off-by: Andrew Jeffery <andrew@...econstruct.com.au>
> > ---
> >  drivers/soc/aspeed/aspeed-lpc-snoop.c | 25 ++++++++++++++++---------
> >  1 file changed, 16 insertions(+), 9 deletions(-)
> > 
> > diff --git a/drivers/soc/aspeed/aspeed-lpc-snoop.c b/drivers/soc/aspeed/aspeed-lpc-snoop.c
> > index ca7536213e0986f737606a52996ffea620df2a7a..804c6ed9c4c671da73a6c66c1de41c59922c82dc 100644
> > --- a/drivers/soc/aspeed/aspeed-lpc-snoop.c
> > +++ b/drivers/soc/aspeed/aspeed-lpc-snoop.c
> > @@ -25,7 +25,6 @@
> >  
> >  #define DEVICE_NAME    "aspeed-lpc-snoop"
> >  
> > -#define NUM_SNOOP_CHANNELS 2
> >  #define SNOOP_FIFO_SIZE 2048
> >  
> >  #define HICR5  0x80
> > @@ -57,6 +56,12 @@ struct aspeed_lpc_snoop_model_data {
> >         unsigned int has_hicrb_ensnp;
> >  };
> >  
> > +enum aspeed_lpc_snoop_index {
> > +       ASPEED_LPC_SNOOP_INDEX_0 = 0,
> > +       ASPEED_LPC_SNOOP_INDEX_1 = 1,
> > +       ASPEED_LPC_SNOOP_INDEX_MAX = ASPEED_LPC_SNOOP_INDEX_1,
> > +};
> 
> I don't have a strong opinion on this (again, I'm neither the driver
> maintainer nor the subsystem maintainer so my opinion has little
> value), but IMHO the main value of introducing an enum here was to make
> it possible to get rid of the default statement in the switch
> constructs. With switch constructs being gone in patch 10/10 (soc:
> aspeed: lpc-snoop: Lift channel config to const structs), the value of
> this enum seems pretty low now. You could use NUM_SNOOP_CHANNELS
> instead of ASPEED_LPC_SNOOP_INDEX_MAX + 1 and 0 and 1 instead of
> ASPEED_LPC_SNOOP_INDEX_0 and ASPEED_LPC_SNOOP_INDEX_1, respectively,
> and the code would work just the same, while being more simple, with no
> downside that I can see.
> 

Yeah, I agonised over it a bit before posting. However, I'm on leave,
and I'd like to draw a line under this series. This patch is in the
middle of it, and I'd rather not disrupt it too much and go around
again with a v3. I'm going to keep the enum for now, but if I need to
tidy up the driver down again the track I'll reconsider its worth.

Andrew