Re: [PATCH v2 1/3] rust: core abstractions for network PHY drivers

[Andrew Lunn <andrew@lunn.ch>]

> > +    /// 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