linux-kernel - Re: [PATCH] spi: armada-3700: fix unsigned compare than zero on irq

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: <47d82067-0a4c-32e9-fe78-16f2e6735aa6@canonical.com>
Date:   Tue, 13 Dec 2016 11:01:52 +0000
From:   Colin Ian King <colin.king@...onical.com>
To:     Romain Perier <romain.perier@...e-electrons.com>,
        Mark Brown <broonie@...nel.org>, linux-spi@...r.kernel.org
Cc:     linux-kernel@...r.kernel.org
Subject: Re: [PATCH] spi: armada-3700: fix unsigned compare than zero on irq

On 13/12/16 10:55, Romain Perier wrote:
> Hello,
> 
> Le 13/12/2016 à 11:28, Colin King a écrit :
>> From: Colin Ian King <colin.king@...onical.com>
>>
>> spi->irq is an unsigned integer hence the check if status is less than
>> zero has no effect.  Fix this by replacing spi->irq with an int irq
>> so the less than zero compare will correctly detect errors.
>>
>> Issue found with static analysis with CoverityScan, CID1388567
>>
>> Signed-off-by: Colin Ian King <colin.king@...onical.com>
>> ---
>>  drivers/spi/spi-armada-3700.c | 9 +++++----
>>  1 file changed, 5 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/spi/spi-armada-3700.c
>> b/drivers/spi/spi-armada-3700.c
>> index e89da0a..4e92178 100644
>> --- a/drivers/spi/spi-armada-3700.c
>> +++ b/drivers/spi/spi-armada-3700.c
>> @@ -800,7 +800,7 @@ static int a3700_spi_probe(struct platform_device
>> *pdev)
>>      struct spi_master *master;
>>      struct a3700_spi *spi;
>>      u32 num_cs = 0;
>> -    int ret = 0;
>> +    int irq, ret = 0;
>>
>>      master = spi_alloc_master(dev, sizeof(*spi));
>>      if (!master) {
>> @@ -846,12 +846,13 @@ static int a3700_spi_probe(struct
>> platform_device *pdev)
>>          goto error;
>>      }
>>
>> -    spi->irq = platform_get_irq(pdev, 0);
>> -    if (spi->irq < 0) {
>> -        dev_err(dev, "could not get irq: %d\n", spi->irq);
>> +    irq = platform_get_irq(pdev, 0);
>> +    if (irq < 0) {
>> +        dev_err(dev, "could not get irq: %d\n", irq);
>>          ret = -ENXIO;
>>          goto error;
>>      }
>> +    spi->irq = irq;
>>
>>      init_completion(&spi->done);
>>
>>
> 
> Why don't you change only the type of "irq" in struct a3700_spi ?
> 
> Thanks,
> Romain

..because I've observed that a lot of driver seems to use a unsigned int
for the irq in their driver specific structs and I also didn't want to
modify this to an int in case it some subtle signed/unsigned logic
somewhere else that I was not aware of. Just seemed like the least risky
fix to me.

Colin