Re: [PATCH v2 1/3] rust: core abstractions for network PHY drivers
From: Andrew Lunn <andrew@lunn.ch>
Date: 2023-10-07 14:48:01
Also in:
rust-for-linux
quoted
+ /// Sets the speed of the PHY. + pub fn set_speed(&mut self, speed: u32) { + let phydev = self.0.get(); + // SAFETY: `phydev` is pointing to a valid object by the type invariant of `Self`. + unsafe { (*phydev).speed = speed as i32 }; + }Since we're taking user input, it probably doesn't hurt to do some sort of sanity check rather than casting. Maybe warn once then return the biggest nowrapping value
After reading the thread, we first have a terminology problem. In the kernel world, 'user input' generally means from user space. And user space should never be trusted, but user space should also not be allowed to bring the system to its knees. Return -EINVAL to userspace is the correct thing to do and keep going. Don't do a kernel splat because the user passed 42 as a speed, not 10. However, what Trevor was meaning is that whoever called set_speed() passed an invalid value. But what are valid values? We have this file which devices the KAPI https://elixir.bootlin.com/linux/latest/source/include/uapi/linux/ethtool.h#L1883 says: /* The forced speed, in units of 1Mb. All values 0 to INT_MAX are legal. and we also have #define SPEED_UNKNOWN -1 and there is a handy little helper: static inline int ethtool_validate_speed(__u32 speed) { return speed <= INT_MAX || speed == (__u32)SPEED_UNKNOWN; } so if you want to validate speed, call this helper. However, this is a kernel driver, and we generally trust kernel drivers. There is not much we can do except trust them. And passing an invalid speed here is very unlikely to cause the kernel to explode sometime later. Andrew