linux-kernel - Re: [PATCH] char: pcmcia: remove set but not used variable 'tmp'

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]

Date:   Sat, 15 May 2021 15:15:56 +0800
From:   "yukuai (C)" <yukuai3@...wei.com>
To:     Arnd Bergmann <arnd@...db.de>
CC:     Harald Welte <laforge@...monks.org>,
        gregkh <gregkh@...uxfoundation.org>, <akpm@...l.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        "zhangyi (F)" <yi.zhang@...wei.com>
Subject: Re: [PATCH] char: pcmcia: remove set but not used variable 'tmp'

On 2021/05/14 14:28, Arnd Bergmann wrote:
> On Fri, May 14, 2021 at 8:21 AM Yu Kuai <yukuai3@...wei.com> wrote:
>>
>> Fixes gcc '-Wunused-but-set-variable' warning:
>>
>> drivers/char/pcmcia/cm4000_cs.c:1053:16: warning: variable ‘tmp’
>> set but not used [-Wunused-but-set-variable]
>>
>> It is never used and so can be removed.
>>
>> Fixes: c1986ee9bea3 ("[PATCH] New Omnikey Cardman 4000 driver")
>> Signed-off-by: Yu Kuai <yukuai3@...wei.com>
> 
> Looks good to me. This was likely written that way at a time when some
> architecture implemented inb() as a macro, and ignoring its value
> would cause a different warning.
> 
> Since you are already touching this file, can you have a look at this
> warning as well:
> 
>     drivers/char/pcmcia/cm4000_cs.c: In function 'set_protocol':
>>> drivers/char/pcmcia/cm4000_cs.c:569:16: warning: iteration 4 invokes undefined behavior [-Waggressive-loop-optimizations]
>       569 |   pts_reply[i] = inb(REG_BUF_DATA(iobase));
>           |   ~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~
>     drivers/char/pcmcia/cm4000_cs.c:567:2: note: within this loop
>       567 |  for (i = 0; i < num_bytes_read; i++) {
> 
> This looks like a preexisting problem that was uncovered by a patch
> that is now in linux-next to change the inb() definition once more,
> I got a report from the kernel build bot about it after I merged the
> patch into the asm-generic tree. It needs a range check on
> num_bytes_read, or a Kconfig check to ensure it is not built on
> architectures without working inb()/outb() operations.
> 
>          Arnd
> .
> 
Hi,

I'm not familar with the logical here, however, if io_read_num_rec_bytes
may get 'num_bytes_read' greater than 4, this loop will cause index out
of boundary. It make sense to me to add a range check. Futhermore, since
'num_bytes_read' is ensure to >= 4，I think we can change the loop to:

for (i = 0; i < 4; ++i) {
	xoutb(i, REG_BUF_ADDR(iobase));
	pts_reply[i] = inb(REG_BUF_DATA(iobase));
}

Thanks
Yu Kuai