netdev - Re: [PATCH v2 1/3] 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]

Message-ID: <96800001-5d19-4b48-b43e-0cfbeccb48c1@lunn.ch>
Date: Sat, 7 Oct 2023 16:47:55 +0200
From: Andrew Lunn <andrew@...n.ch>
To: Trevor Gross <tmgross@...ch.edu>
Cc: FUJITA Tomonori <fujita.tomonori@...il.com>, netdev@...r.kernel.org,
	rust-for-linux@...r.kernel.org, miguel.ojeda.sandonis@...il.com,
	greg@...ah.com
Subject: Re: [PATCH v2 1/3] rust: core abstractions for network PHY drivers

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