linux-kernel - Re: [PATCH 1/4] scsi: pmcraid: use _

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

Date:   Sat, 22 Apr 2017 00:01:19 +0200
From:   Arnd Bergmann <arnd@...db.de>
To:     Al Viro <viro@...iv.linux.org.uk>
Cc:     "James E.J. Bottomley" <jejb@...ux.vnet.ibm.com>,
        "Martin K. Petersen" <martin.petersen@...cle.com>,
        Johannes Berg <johannes.berg@...el.com>,
        linux-scsi@...r.kernel.org,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 1/4] scsi: pmcraid: use __iomem pointers for ioctl argument

On Thu, Apr 20, 2017 at 9:24 PM, Al Viro <viro@...iv.linux.org.uk> wrote:
> On Thu, Apr 20, 2017 at 07:54:45PM +0200, Arnd Bergmann wrote:
>> kernelci.org reports a new compile warning for old code in the pmcraid
>> driver:
>>
>> arch/mips/include/asm/uaccess.h:138:21: warning: passing argument 1 of '__access_ok' makes pointer from integer without a cast [-Wint-conversion]
>>
>> The warning got introduced by a cleanup to the access_ok() helper
>> that requires the argument to be a pointer, where the old version
>> silently accepts 'unsigned long' arguments as it still does on most
>> other architectures.
>>
>> The new behavior in MIPS however seems absolutely sensible, and so far I
>> could only find one other file with the same issue, so the best solution
>> seems to be to clean up the pmcraid driver.
>>
>> This makes the driver consistently use 'void __iomem *' pointers for
>> passing around the address of the user space ioctl arguments, which gets
>> rid of the kernelci warning as well as several sparse warnings.
>
> Is there any point in keeping that access_ok() in the first place, rather
> than just switching to copy_from_user()/copy_to_user() in there?  AFAICS,
> it's only for the sake of the loop in pmcraid_copy_sglist():
>         for (i = 0; i < (len / bsize_elem); i++, buffer += bsize_elem) {
>                 struct page *page = sg_page(&scatterlist[i]);
>
>                 kaddr = kmap(page);
>                 if (direction == DMA_TO_DEVICE)
>                         rc = __copy_from_user(kaddr,
>                                               (void *)buffer,
>                                               bsize_elem);
>                 else
>                         rc = __copy_to_user((void *)buffer, kaddr, bsize_elem);
>
>                 kunmap(page);
>
>                 if (rc) {
>                         pmcraid_err("failed to copy user data into sg list\n");
>                         return -EFAULT;
>                 }
>
>                 scatterlist[i].length = bsize_elem;
>         }
> and seeing that each of those calls copies is at least a full page...  If
> there is an architecture where a single access_ok() costs a noticable fraction
> of the time it takes to copy a full page, we have a much worse problem than
> overhead in obscure ioctl...

Right, that would also fix the warning. I think we should just do both
fixes, as they are each a worthwhile cleanup. I can do this as another
patch on top of the series.

I've done that second patch now and given it a spin on the randconfig test
builds.

       Arnd