netdev - Re: [PATCH net-next v5 1/5] rust: core abstractions for network PHY drivers

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, 21 Oct 2023 13:35:57 +0000
From: Benno Lossin <benno.lossin@...ton.me>
To: FUJITA Tomonori <fujita.tomonori@...il.com>
Cc: netdev@...r.kernel.org, rust-for-linux@...r.kernel.org, andrew@...n.ch, miguel.ojeda.sandonis@...il.com, tmgross@...ch.edu, boqun.feng@...il.com, wedsonaf@...il.com, greg@...ah.com
Subject: Re: [PATCH net-next v5 1/5] rust: core abstractions for network PHY drivers

On 21.10.23 15:31, FUJITA Tomonori wrote:
> On Sat, 21 Oct 2023 13:05:59 +0000
> Benno Lossin <benno.lossin@...ton.me> wrote:
> 
>> On 21.10.23 15:00, FUJITA Tomonori wrote:
>>> On Sat, 21 Oct 2023 12:50:10 +0000
>>> Benno Lossin <benno.lossin@...ton.me> wrote:
>>>>>>>> I think this is very weird, do you have any idea why this
>>>>>>>> could happen?
>>>>>>>
>>>>>>> DriverVtable is created on kernel stack, I guess.
>>>>>>
>>>>>> But how does that invalidate the function pointers?
>>>>>
>>>>> Not only funciton pointers. You can't store something on stack for
>>>>> later use.
>>>>
>>>> It is not stored on the stack, it is only created on the stack and
>>>> moved to a global static later on. The `module!` macro creates a
>>>> `static mut __MOD: Option<Module>` where the module data is stored in.
>>>
>>> I know. The problem is that we call phy_drivers_register() with
>>> DriverVTable on stack. Then it was moved.
>>
>> I see, what exactly is the problem with that? In other words:
>> why does PHYLIB need `phy_driver` to stay at the same address?
> 
> phy_driver_register stores addresses that you passed.

But the function pointers don't change?

>> This is an important requirement in Rust. Rust can ensure that
>> types are not moved by means of pinning them. In this case, Wedson's
>> patch below should fix the issue completely.
>>
>> But we should also fix this in the abstractions, the `DriverVTable`
>> type should only be constructible in a pinned state. For this purpose
>> we have the `pin-init` API [2].
> 
> You can create DriverVTable freely. The restriction is what
> phy_driver_register takes. 

Sure you can also keep it this way, I just thought only allowing
pinned creation makes things simpler.

> Currently, it needs &'static DriverVTable
> array so it works.

That is actually also incorrect. As the C side is going to modify
the `DriverVTable`, you should actually use `&'static mut DriverVTable`.
But since it is not allowed to be moved you have to use
`Pin<&'static mut DriverVTable>`.

> The C side uses static allocation too. If someone asks for, we could
> loosen the restriction with a complicated implentation. But I doubt
> that someone would ask for such.

With Wedson's patch you also would be using the static allocation
from `module!`. What my problem is, is that you are using a `static mut`
which is `unsafe` and you do not actually have to use it (with
Wedson's patch of course).

-- 
Cheers,
Benno