linux-kernel - Re: [PATCH] staging: comedi: serial2002: bare unsigned and unneeded cast styling issues

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 for Android: free password hash cracker in your pocket

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

Message-ID: <5759369A.2060509@mev.co.uk>
Date:	Thu, 9 Jun 2016 10:27:54 +0100
From:	Ian Abbott <abbotti@....co.uk>
To:	Chris Cesare <chris.cesare@...il.com>
Cc:	devel@...verdev.osuosl.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] staging: comedi: serial2002: bare unsigned and unneeded
 cast styling issues

On 08/06/16 20:59, Chris Cesare wrote:
> checkpatch.pl reported two warnings: A bare "unsigned" and an
> unnecessary cast. Fixed both.
>
> Signed-off-by: Chris Cesare <chris.cesare@...il.com>
> ---
>   drivers/staging/comedi/drivers/serial2002.c | 5 +++--
>   1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/staging/comedi/drivers/serial2002.c b/drivers/staging/comedi/drivers/serial2002.c
> index 7a1defc..d3c2743 100644
> --- a/drivers/staging/comedi/drivers/serial2002.c
> +++ b/drivers/staging/comedi/drivers/serial2002.c
> @@ -95,7 +95,7 @@ struct serial_data {
>   #define S2002_CFG_SIGN(x)		(((x) >> 13) & 0x1)
>   #define S2002_CFG_BASE(x)		(((x) >> 14) & 0xfffff)
>
> -static long serial2002_tty_ioctl(struct file *f, unsigned op,
> +static long serial2002_tty_ioctl(struct file *f, unsigned int op,
>   				 unsigned long param)
>   {
>   	if (f->f_op->unlocked_ioctl)
> @@ -379,7 +379,8 @@ static int serial2002_setup_subdevice(struct comedi_subdevice *s,
>   				range_table_list[chan] =
>   				    (const struct comedi_lrange *)&range[j];
>   			}
> -			maxdata_list[chan] = ((long long)1 << cfg[j].bits) - 1;
> +			maxdata_list[chan] = (1 << cfg[j].bits) - 1;
> +
>   			chan++;
>   		}
>   	}
>

Logically, the cfg[j].bits value can range from 0 to 63.  Practically, I 
guess it has some fixed set of non-zero values up to a maximum of 32.  A 
shift value of 32 is too large for both a plain, signed int, and an 
unsigned int (assuming 32-bit int), so the change is a bit dodgy.

I propose splitting the patch into 2, fixing the bare unsigned in patch 
1, and fixing the cast styling issue in patch 2.  Since 
maxdata_list[chan] is only 32 bits wide, the safest change would be 
something like this, clamping the value if cfg[j].bits happens to be 
larger than 32:

			if (cfg[j].bits < 32)
				maxdata_list[chan] = (1u << cfg[j].bits) - 1;
			else
				maxdata_list[chan] = 0xffffffff;

-- 
-=( Ian Abbott @ MEV Ltd.    E-mail: <abbotti@....co.uk> )=-
-=(                          Web: http://www.mev.co.uk/  )=-