linux-kernel - Re: [PATCH 06/16] char: applicom: Remove 3 unused variables 'ret' and 2 instances of 'byte_reset

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:   Thu, 20 May 2021 14:27:57 +0200
From:   Arnd Bergmann <arnd@...db.de>
To:     Lee Jones <lee.jones@...aro.org>
Cc:     Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        David Woodhouse <dwmw2@...radead.org>
Subject: Re: [PATCH 06/16] char: applicom: Remove 3 unused variables 'ret' and
 2 instances of 'byte_reset_it'

On Thu, May 20, 2021 at 2:13 PM Lee Jones <lee.jones@...aro.org> wrote:

> @@ -568,7 +565,7 @@ static ssize_t ac_read (struct file *filp, char __user *buf, size_t count, loff_
>
>                                 /* Got a packet for us */
>                                 memset(&st_loc, 0, sizeof(st_loc));
> -                               ret = do_ac_read(i, buf, &st_loc, &mailbox);
> +                               do_ac_read(i, buf, &st_loc, &mailbox);
>                                 spin_unlock_irqrestore(&apbs[i].mutex, flags);
>                                 set_current_state(TASK_RUNNING);
>                                 remove_wait_queue(&FlagSleepRec, &wait);

This change doesn't look right to me. It's clear that the code we have is bogus,
but I think what the authors intended was:

--- a/drivers/char/applicom.c
+++ b/drivers/char/applicom.c
@@ -577,7 +577,7 @@ static ssize_t ac_read (struct file *filp, char
__user *buf, size_t count, loff_
                                        return -EFAULT;
                                if (copy_to_user(buf + sizeof(st_loc),
&mailbox, sizeof(mailbox)))
                                        return -EFAULT;
-                               return tmp;
+                               return ret;
                        }

                        if (tmp > 2) {

>From the git history, I can see that this got changed in linux-2.3.99,
but the previous
version just returned zero on success instead of the number of bytes,
so presumably
nobody ever cared about the returned value.

> @@ -700,7 +697,6 @@ static long ac_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
>         void __iomem *pmem;
>         int ret = 0;
>         static int warncount = 10;
> -       volatile unsigned char byte_reset_it;
>         struct st_ram_io *adgl;
>         void __user *argp = (void __user *)arg;
>
> @@ -762,7 +758,7 @@ static long ac_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
>
>                 for (i = 0; i < MAX_BOARD; i++) {
>                         if (apbs[i].RamIO) {
> -                               byte_reset_it = readb(apbs[i].RamIO + RAM_IT_TO_PC);
> +                               readb(apbs[i].RamIO + RAM_IT_TO_PC);
>                         }

This is a little subtle but should be fine on all architectures now. I suspect
this used to be required a long time ago before the readb() function worked
correctly everywhere.

I'm also reminded that I had meant to send a patch to move this file
to drivers/staging,
as all evidence suggests it has not been used for decades.

       Arnd