Re: [TECH TOPIC] Rust for Linux

[Wedson Almeida Filho <wedsonaf@google.com>]

On Tue, Jul 20, 2021 at 12:16:32AM +0200, Linus Walleij wrote:
> On Mon, Jul 19, 2021 at 4:42 PM Wedson Almeida Filho
> <wedsonaf@google.com> wrote:
> 
> > > I think people want to see the code inside these classes as well,
> > > can we browse it?
> >
> > The whole tree is available here: https://github.com/wedsonaf/linux/tree/pl061
> > -- I should caution you that the two most recent commits are not merged into the
> > rust tree yet because they're WIP. (I'll clean them up and eventually merge
> > after the feedback from you all.)
> 
> I found that the gpio_chip C wrappers lives here:
> https://github.com/wedsonaf/linux/blob/pl061/rust/kernel/gpio.rs
> 
> This file is very interesting!
> 
> I think it's not a good idea to keep these wrappers in their own
> rust directory, they need to be distributed out into the kernel
> everywhere they are used. We have made this mistake before
> with OF (device tree) that started exactly like this in
> drivers/of/*, and now I have part of the OF GPIO handling
> and tests inside files in that directory as a consequence.

That's great feedback. Our plan was to have the core parts in `rust/kernel`, but
once maintainers got involved, we would place things where it made more sense.
Since we have no maintainers involved in development yet, everything is in rust/
for now.

> The wrappers are a hard read, I notice a whole lot of unsafe
> keywords here, Chip is missing .set_config() which is understandable
> since PL061 isn't using it,

Right, I only implemented what was needed for the PL061 driver. As I said
originally, this isn't intended to be a general solution just yet.

> the New call to create an instance
> contains a lot of magic stuff I don't understand like
> state: AtomicU8::new(0) and such but I think that's all right
> it probably only needs a trained eye.

That code is interacting with C, both being called by C and calling into C, so
it necessarily has a lot of unsafe blocks. The idea though is to limit the
unsafety to these wrappers and have them carefully reviewed; users will
[ideally] need no unsafe code.

> This binding only concerns the gpio_chip abstraction though,
> which means that the struct gpio_device and the creation and
> handling of the GPIO character device, all of the ioctl()
> and passed in/out data from userspace is left in C in gpiolib.c

Right, we don't want to reinvent things already in C while trying to demonstrate
a driver. The wrappers I wrote are needed for us to have a safe Rust API that
drivers can call -- if they weren't necessary I wouldn't have written them
either. At least not to begin with.

> I think this is the current ambition of the Rust effort for now,
> which is fine, one can not be judged for doing what one
> attempts to do.
> 
> But we need to acknowledge that by leaving the userspace
> facing code in C the attack surface for buffer overruns
> etc is left in C and it buys us pretty much zero additional
> security. And with security I do not mean the kernel
> protecting itself from itself, but from things coming from
> userspace.
>
> It's not that I think GPIOs are an especially interesting
> attack vector, but if this pattern persists then it becomes
> a problem, because Rust isn't yet protecting us (GPIO)
> against userspace, which is what it needs to do in the end.

It's not protecting all the way yet, but it is offering more guarantees than C:
for example, it guarantees that driver code won't touch memory that it doesn't
own. Suppose there's a bug in gpiolib that allows an attacker to control the
value of `offset` in get/set operations, a driver written in C will take it and
read/write to memory it doesn't own; a Rust driver won't.

Suppose a driver developer forgets to call chained_irq_enter/chained_irq_exit in
their IRQ handler. An attacker could time things and trigger interrupts and DoS
the system, but this can't happen in Rust.

Suppose a driver developer copies some code from pl061_irq_type and pastes it
somewhere where irq_desc is not locked, but calls irq_set_handler_locked anyway.
An attacker could potentially exploit the resulting races. This can't happen in
Rust.

There are other bugs that Rust prevents by construction that improves the
overall quality of the drivers, with the end result being a reduced attack
surface. Less opportunities for attackers to chain vulnerabilities to exploit
the kernel -- remember that exploits are usually built as chain of
vulnerabilities to elevate from simple innocuous-looking misbehaviours to
something really bad like arbitrary execution.

> What we get now is the ability to write drivers in Rust,
> but no protection against a hostile userspace. Not really.

I do agree that having Rust on the interface with userspace would likely offer
the best protection. That's what I was working on with binder -- but you (as a
community) wanted to see a driver for "real" hardware. So here it is :)

> To that end the core of the GPIO library would probably
> have to be rewritten in Rust and used on *all* platforms
> in order to buy us any improved security. I understand that
> this is not currently the plan.

It's not the short-term plan, but if maintainers are interested in converting
GPIO to Rust, I would be happy to help now that I've learned a bit about it.
It would, for one, fix the race conditions you have when devices are removed.

> This is what I have said before as well: Rust needs to go
> where the attack surface of the kernel is to buy us any
> real security. But getting there will require rewriting entire
> subsystems in Rust AFAICT.

I'm not sure I agree with this. Incremental improvements are a good thing.

Cheers,
-Wedson