linux-kernel - Re: [PATCH] staging: vchiq: Avoid mixing bulk

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

Message-Id: <3d3b7368-93b2-4c0d-845e-4099c2de9dc1@app.fastmail.com>
Date: Tue, 30 Jul 2024 23:28:40 +0200
From: "Arnd Bergmann" <arnd@...db.de>
To: "Stefan Wahren" <wahrenst@....net>,
 "Umang Jain" <umang.jain@...asonboard.com>, linux-staging@...ts.linux.dev
Cc: "Greg Kroah-Hartman" <gregkh@...uxfoundation.org>,
 "Florian Fainelli" <florian.fainelli@...adcom.com>,
 "Ray Jui" <rjui@...adcom.com>, "Scott Branden" <sbranden@...adcom.com>,
 "Dan Carpenter" <dan.carpenter@...aro.org>,
 "Kieran Bingham" <kieran.bingham@...asonboard.com>,
 "laurent.pinchart" <laurent.pinchart@...asonboard.com>,
 "Yang Li" <yang.lee@...ux.alibaba.com>,
 "Wolfram Sang" <wsa+renesas@...g-engineering.com>,
 linux-rpi-kernel@...ts.infradead.org, linux-arm-kernel@...ts.infradead.org,
 linux-kernel@...r.kernel.org
Subject: Re: [PATCH] staging: vchiq: Avoid mixing bulk_userdata kernel and userspace
 pointer

On Tue, Jul 30, 2024, at 20:05, Stefan Wahren wrote:
> Hi Umang,
>
> Am 30.07.24 um 19:08 schrieb Umang Jain:
>> In vchiq_dev.c, there are two places where the __user bulk_userdata
>> pointer to set to a kernel-space pointer which then gives relevant
>> Sparse warnings as below:
>>
>> vchiq_dev.c:328:26: warning: incorrect type in assignment (different address spaces)
>> vchiq_dev.c:328:26:    expected void *[assigned] userdata
>> vchiq_dev.c:328:26:    got void [noderef] __user *userdata
>> vchiq_dev.c:543:47: warning: incorrect type in assignment (different address spaces)
>> vchiq_dev.c:543:47:    expected void [noderef] __user *[addressable] [assigned] bulk_userdata
>> vchiq_dev.c:543:47:    got void *bulk_userdata
>>
>> This is solved by adding additional functional argument to track the
>> userspace bulk_userdata separately and passing it accordingly to
>> completion handlers.
> IMO this patch fixes the issue for spare, but don't address the
> confusing member naming for humans. It's not clear that "userdata" is a
> kernel pointer and "uuserdata" is a pointer to userspace. It would be
> nice to avoid the word "user" for kernel pointer in this case.

Right, also you need to provide a much better explanation about
how the code is meant to work, and what this opaque pointer
is meant to do.

Ideally this should be cleaned up in a way that completely
avoids passing both user and kernel data at the same time.

A small step would be to separate out the "struct bulk_waiter
*bulk_waiter" argument and make that typesafe.

You can also wrap vchiq_bulk_transfer() in order to have
four separate functions based on the different 'mode'
values and have them only take the arguments they actually
need.

    Arnd