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
| ||
|
Message-ID: <ZT6fzfV9GUQOZnlx@boqun-archlinux> Date: Sun, 29 Oct 2023 11:09:17 -0700 From: Boqun Feng <boqun.feng@...il.com> To: FUJITA Tomonori <fujita.tomonori@...il.com> Cc: benno.lossin@...ton.me, andrew@...n.ch, netdev@...r.kernel.org, rust-for-linux@...r.kernel.org, tmgross@...ch.edu, miguel.ojeda.sandonis@...il.com, wedsonaf@...il.com Subject: Re: [PATCH net-next v7 1/5] rust: core abstractions for network PHY drivers On Sun, Oct 29, 2023 at 09:48:41AM -0700, Boqun Feng wrote: > On Sun, Oct 29, 2023 at 01:21:12PM +0900, FUJITA Tomonori wrote: > [...] > > > > The current code is fine from Rust perspective because the current > > code copies phy_driver on stack and makes a reference to the copy, if > > I undertand correctly. > > > > I had the same thought Benno brought the issue on `&`, but unfortunately > it's not true ;-) In the following code: > > let phydev = unsafe { *self.0.get() }; > > , semantically the *whole* `bindings::phy_device` is being read, so if > there is any modification (i.e. write) that may happen in the meanwhile, > it's data race, and data races are UB (even in C). > > So both implementations have the problem because of the same cause. > > > It's not nice to create an 500-bytes object on stack. It turned out > > that it's not so simple to avoid it. > > As you can see, copying is not the way to work around this. > An temporary solution is doing the #2 option from Benno, but in Rust and open code it, like the following: diff --git a/rust/kernel/net/phy.rs b/rust/kernel/net/phy.rs index 145d0407fe31..f5230ac48014 100644 --- a/rust/kernel/net/phy.rs +++ b/rust/kernel/net/phy.rs @@ -121,10 +121,10 @@ pub fn state(&self) -> DeviceState { /// /// It returns true if the link is up. pub fn is_link_up(&self) -> bool { - const LINK_IS_UP: u32 = 1; + const LINK_IS_UP: u64 = 1; // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`. - let phydev = unsafe { *self.0.get() }; - phydev.link() == LINK_IS_UP + let bit_field = unsafe { &(*self.0.get())._bitfield_1 }; + bit_field.get(14, 1) == LINK_IS_UP } /// Gets the current auto-negotiation configuration. @@ -132,18 +132,18 @@ pub fn is_link_up(&self) -> bool { /// It returns true if auto-negotiation is enabled. pub fn is_autoneg_enabled(&self) -> bool { // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`. - let phydev = unsafe { *self.0.get() }; - phydev.autoneg() == bindings::AUTONEG_ENABLE + let bit_field = unsafe { &(*self.0.get())._bitfield_1 }; + bit_field.get(13, 1) == bindings::AUTONEG_ENABLE as u64 } /// Gets the current auto-negotiation state. /// /// It returns true if auto-negotiation is completed. pub fn is_autoneg_completed(&self) -> bool { - const AUTONEG_COMPLETED: u32 = 1; + const AUTONEG_COMPLETED: u64 = 1; // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`. - let phydev = unsafe { *self.0.get() }; - phydev.autoneg_complete() == AUTONEG_COMPLETED + let bit_field = unsafe { &(*self.0.get())._bitfield_1 }; + bit_field.get(15, 1) == AUTONEG_COMPLETED } /// Sets the speed of the PHY. Of course, it's not maintainable in longer term since it relies on hard-coding the bit offset of these bit fields. But I think it's best we can do from Linux kernel side. It's up to Andrew and Miguel whether this temporary solution is OK. Regards, Boqun
Powered by blists - more mailing lists