| 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
| ||
|
Message-ID: <2024120551-sternness-wilder-f417@gregkh>
Date: Thu, 5 Dec 2024 10:47:19 +0100
From: Greg KH <gregkh@...uxfoundation.org>
To: Lee Jones <lee@...nel.org>
Cc: linux-kernel@...r.kernel.org, ojeda@...nel.org, alex.gaynor@...il.com,
boqun.feng@...il.com, gary@...yguo.net, bjorn3_gh@...tonmail.com,
benno.lossin@...ton.me, a.hindborg@...nel.org, aliceryhl@...gle.com,
tmgross@...ch.edu, rust-for-linux@...r.kernel.org
Subject: Re: [PATCH 2/2] sample: rust_misc_device: Demonstrate additional
get/set value functionality
On Thu, Dec 05, 2024 at 09:27:06AM +0000, Lee Jones wrote:
> On Thu, 05 Dec 2024, Greg KH wrote:
>
> > On Thu, Dec 05, 2024 at 08:38:48AM +0000, Lee Jones wrote:
> > > On Wed, 04 Dec 2024, Greg KH wrote:
> > >
> > > > On Wed, Dec 04, 2024 at 05:46:25PM +0000, Lee Jones wrote:
> > > > > Expand the complexity of the sample driver by providing the ability to
> > > > > get and set an integer. The value is protected by a mutex.
> > > > >
> > > > > Here is a simple userspace program that fully exercises the sample
> > > > > driver's capabilities.
> > > > >
> > > > > int main() {
> > > > > int value, new_value;
> > > > > int fd, ret;
> > > > >
> > > > > // Open the device file
> > > > > printf("Opening /dev/rust-misc-device for reading and writing\n");
> > > > > fd = open("/dev/rust-misc-device", O_RDWR);
> > > > > if (fd < 0) {
> > > > > perror("open");
> > > > > return errno;
> > > > > }
> > > > >
> > > > > // Make call into driver to say "hello"
> > > > > printf("Calling Hello\n");
> > > > > ret = ioctl(fd, RUST_MISC_DEV_HELLO, NULL);
> > > > > if (ret < 0) {
> > > > > perror("ioctl: Failed to call into Hello");
> > > > > close(fd);
> > > > > return errno;
> > > > > }
> > > > >
> > > > > // Get initial value
> > > > > printf("Fetching initial value\n");
> > > > > ret = ioctl(fd, RUST_MISC_DEV_GET_VALUE, &value);
> > > > > if (ret < 0) {
> > > > > perror("ioctl: Failed to fetch the initial value");
> > > > > close(fd);
> > > > > return errno;
> > > > > }
> > > > >
> > > > > value++;
> > > > >
> > > > > // Set value to something different
> > > > > printf("Submitting new value (%d)\n", value);
> > > > > ret = ioctl(fd, RUST_MISC_DEV_SET_VALUE, &value);
> > > > > if (ret < 0) {
> > > > > perror("ioctl: Failed to submit new value");
> > > > > close(fd);
> > > > > return errno;
> > > > > }
> > > > >
> > > > > // Ensure new value was applied
> > > > > printf("Fetching new value\n");
> > > > > ret = ioctl(fd, RUST_MISC_DEV_GET_VALUE, &new_value);
> > > > > if (ret < 0) {
> > > > > perror("ioctl: Failed to fetch the new value");
> > > > > close(fd);
> > > > > return errno;
> > > > > }
> > > > >
> > > > > if (value != new_value) {
> > > > > printf("Failed: Committed and retrieved values are different (%d - %d)\n", value, new_value);
> > > > > close(fd);
> > > > > return -1;
> > > > > }
> > > > >
> > > > > // Call the unsuccessful ioctl
> > > > > printf("Attempting to call in to an non-existent IOCTL\n");
> > > > > ret = ioctl(fd, RUST_MISC_DEV_FAIL, NULL);
> > > > > if (ret < 0) {
> > > > > perror("ioctl: Succeeded to fail - this was expected");
> > > > > } else {
> > > > > printf("ioctl: Failed to fail\n");
> > > > > close(fd);
> > > > > return -1;
> > > > > }
> > > > >
> > > > > // Close the device file
> > > > > printf("Closing /dev/rust-misc-device\n");
> > > > > close(fd);
> > > > >
> > > > > printf("Success\n");
> > > > > return 0;
> > > > > }
> > > > >
> > > > > Signed-off-by: Lee Jones <lee@...nel.org>
> > > > > ---
> > > > > samples/rust/rust_misc_device.rs | 82 ++++++++++++++++++++++++--------
> > > > > 1 file changed, 62 insertions(+), 20 deletions(-)
> > > > >
> > > > > diff --git a/samples/rust/rust_misc_device.rs b/samples/rust/rust_misc_device.rs
> > > > > index 5f1b69569ef7..9c041497d881 100644
> > > > > --- a/samples/rust/rust_misc_device.rs
> > > > > +++ b/samples/rust/rust_misc_device.rs
> > > > > @@ -2,13 +2,20 @@
> > > > >
> > > > > //! Rust misc device sample.
> > > > >
> > > > > +use core::pin::Pin;
> > > > > +
> > > > > use kernel::{
> > > > > c_str,
> > > > > - ioctl::_IO,
> > > > > + ioctl::{_IO, _IOC_SIZE, _IOR, _IOW},
> > > > > miscdevice::{MiscDevice, MiscDeviceOptions, MiscDeviceRegistration},
> > > > > + new_mutex,
> > > > > prelude::*,
> > > > > + sync::Mutex,
> > > > > + uaccess::{UserSlice, UserSliceReader, UserSliceWriter},
> > > > > };
> > > > >
> > > > > +const RUST_MISC_DEV_GET_VALUE: u32 = _IOR::<i32>('R' as u32, 7);
> > > > > +const RUST_MISC_DEV_SET_VALUE: u32 = _IOW::<i32>('R' as u32, 8);
> > > >
> > > > Shouldn't this be 'W'?
> > >
> > > No, I don't think so.
> > >
> > > 'W' doesn't mean 'write'. It's supposed to be a unique identifier:
> > >
> > > 'W' 00-1F linux/watchdog.h conflict!
> > > 'W' 00-1F linux/wanrouter.h conflict! (pre 3.9)
> > > 'W' 00-3F sound/asound.h conflict!
> > > 'W' 40-5F drivers/pci/switch/switchtec.c
> > > 'W' 60-61 linux/watch_queue.h
> > >
> > > 'R' isn't registered for this either:
> > >
> > > 'R' 00-1F linux/random.h conflict!
> > > 'R' 01 linux/rfkill.h conflict!
> > > 'R' 20-2F linux/trace_mmap.h
> > > 'R' C0-DF net/bluetooth/rfcomm.h
> > > 'R' E0 uapi/linux/fsl_mc.h
> > >
> > > ... but since this is just example code with no real purpose, I'm going
> > > to hold short of registering a unique identifier for it.
> >
> > Ah, sorry, I missed that this is the ioctl "name". As the ptrace people
> > will complain, why not use a new one? Ick, ioctl-number.rst is way out
> > of date, but I guess we should carve out one for "sample drivers, do not
> > use in anything real" use cases like here.
>
> I can carve one out for Samples.
>
> > > > > + fn get_value(&self, mut writer: UserSliceWriter) -> Result<isize> {
> > > > > + let guard = self.inner.lock();
> > > > > +
> > > > > + pr_info!("-> Copying data to userspace (value: {})\n", &guard.value);
> > > > > +
> > > > > + writer.write::<i32>(&guard.value)?;
> > > >
> > > > What happens if it fails, shouldn't your pr_info() happen after this?
> > >
> > > If this fails, I need the line in the log to show where it failed.
> >
> > pr_info() doesn't show file lines from what I remember has that changed?
> >
> > But wait, this is a misc device, you should be using dev_info() and
> > friends here, no pr_*() stuff please.
>
> Looks like no other Rust driver does this, so I would be the first.
Yes, because those macros just got added this release cycle :)
Also, moving the existing in-tree drivers to use these macros would be a
good idea.
> I need to figure out how to obtain Device data via these APIs.
You should have access to it from the misc device structure.
> Can I put it on the subsequent work priority list _before_ read/write?
It should be in patch 1 :)
> > > It says "copying" as in "attempting to copy", rather than "copied".
> >
> > Fair enough, but if the copy fails, nothing gets printed out, right?
>
> Not from the kernel, but userspace catches it without issue:
>
> ioctl: Failed to submit new value: Bad address
>
> This does not appear to be common practice in kernel Rust.
>
> The commonly used ? operator seems to just propagate.
Ok, nice!
automatic error handling is going to be one of the killer features here.
greg k-h
Powered by blists - more mailing lists