linux-kernel - Re: [PATCH] staging: vme_user: Change slot number type from int to u32

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>] [day] [month] [year] [list]

Message-ID: <6ba02745-0a55-4f5e-a75a-a7dfbd7070a8@stanley.mountain>
Date: Mon, 26 Aug 2024 15:31:12 +0300
From: Dan Carpenter <dan.carpenter@...aro.org>
To: Riyan Dhiman <riyandhiman14@...il.com>
Cc: gregkh@...uxfoundation.org, linux-kernel@...r.kernel.org,
	linux-staging@...ts.linux.dev
Subject: Re: [PATCH] staging: vme_user: Change slot number type from int to
 u32

On Mon, Aug 26, 2024 at 05:13:25PM +0530, Riyan Dhiman wrote:
> >
> >
> > Of course, there is a place for unsigned types in C but it's so subtle and
> > complicated to explain.  I think people wish that there was a way to make C
> > safer when there really isn't.  There is no easy answer like just declare
> > everything as u32.  It's a false hope.
> >
> 
> The reason I changed int to u32 is that the slot function was declared this
> way.
> 
> static int tsi148_slot_get(struct vme_bridge *tsi148_bridge)
> {
> u32 slot = 0;
> struct tsi148_driver *bridge;
> 
> bridge = tsi148_bridge->driver_priv;
> 
> if (!geoid) {
> slot = ioread32be(bridge->base + TSI148_LCSR_VSTAT);
> slot = slot & TSI148_LCSR_VSTAT_GA_M;
> } else {
> slot = geoid;
> }
> 
> return (int)slot;
> }
> 
> The slot is taken from the ioread32be function, which returns a u32.
> However, it gets cast to an int on return.
> If the value exceeds the int range, it could result in a strange large
> value. I didn't realize the potential bugs
> this could cause. How should cases like this be handled? Should we include
> an if condition to check if the
> slot is within the int range, something like this?
> 
> if (slot > INT_MAX) {
>     return -ERANGE;
> }
> return (int)slot;
> 
> Also, since slot will always return u32 isn't it better to declare it as
> u32 rather than int?
> 

slot is "slot = slot & TSI148_LCSR_VSTAT_GA_M;" which means it is in the 0-31
range.  The "geoid" variable is a module parameter.  These days we frown on
module parameters, and say it should be configured via sysfs instead.
It doesn't make sense for geiod to be negative or >= VME_MAX_SLOTS.  We should
check that in the probe() function.

I would declare geoid as u32, to communicate that negatives are not allowed.

Remove the cast from tsi148_slot_get() but leave the function as returning int.
No need to over engineer it.  Plus that way it matches the type of
vme_slot_num() which returns negative error codes so int is the correct type.

regards,
dan carpenter