On Sat, Oct 7, 2023 at 10:48 AM Andrew Lunn [off-list ref] wrote:

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.

Yes, sorry about the terminology. I was indeed referring to a user of
this function and am still figuring out the failure modes (thanks also
Greg for the corrections).

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

I didn't mean the suggestion to be a way of handling untrusted input,
just a means of providing a fallback value with module author feedback
if something > INT_MAX winds up getting passed somehow.

Agreed that this is more than necessary for such a trivial situation,
the original is of course fine.