Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider: - [High] sysfs: NULL pointer dereference in cpu_show() during mdev probe - [Low] kconfig: ARM_CLA_MDEV linkage error when VFIO or KVM are modules - [Low] char: cla_mdev_ioctl() returns -EINVAL instead of -ENOTTY for unknown ioctls --
commit dd12438e45e5acb03e5d810526fd148e9b134169 Author: Jean-Philippe Brucker <[email protected]> misc/arm-cla: Add vfio-mdev user interface This commit implements an interface to assign CLA devices to KVM guests using the VFIO mediated device framework. It sets up the stage-2 MTC for the assigned VMs and exposes a sysfs interface for device discovery. > diff --git a/drivers/misc/arm-cla/Kconfig b/drivers/misc/arm-cla/Kconfig > index f8436a12c1e64..5def727b7a342 100644 > --- a/drivers/misc/arm-cla/Kconfig > +++ b/drivers/misc/arm-cla/Kconfig > @@ -8,3 +8,19 @@ config ARM_CLA > accelerators. It is only accessible from its associated CPU. > > If unsure, say N. > + > +config ARM_CLA_MDEV > + bool "Mediated device for Arm CLA" > + default y > + depends on KVM > + depends on VFIO [Severity: Low] Will this cause linker errors if ARM_CLA is built-in but KVM or VFIO are built as modules? Because ARM_CLA_MDEV is a bool depending on tristate symbols (KVM, VFIO), it evaluates to y even when KVM or VFIO are m. If ARM_CLA=y, cla-mdev.o is compiled into the kernel. Since cla-mdev.c references module symbols from vfio and kvm, this can result in undefined reference errors during the kernel build. Should the Kconfig dependencies enforce that if ARM_CLA is built-in, its tristate dependencies must also be built-in? > diff --git a/drivers/misc/arm-cla/cla-mdev.c b/drivers/misc/arm-cla/cla-mdev.c > new file mode 100644 > index 0000000000000..66c5dd197542e > --- /dev/null > +++ b/drivers/misc/arm-cla/cla-mdev.c [ ... ] > +static ssize_t cpu_show(struct device *dev, struct device_attribute *attr, > + char *buf) > +{ > + struct cla_mdev *mdev = dev_get_drvdata(dev); > + > + return sysfs_emit(buf, "%u\n", mdev->dev->cpu); [Severity: High] Can this result in a NULL pointer dereference during device probe? The cla_mdev_driver defines dev_groups = cla_mdev_sysfs_groups, meaning the sysfs core creates the cpu file before cla_mdev_probe() is called. If userspace reads the newly created file concurrently, cpu_show() executes before the drvdata is set later in cla_mdev_probe(): static int cla_mdev_probe(struct mdev_device *mdev) { ... dev_set_drvdata(&mdev->dev, cla_mdev); This would cause dev_get_drvdata() to return NULL, leading to a crash when accessing mdev->dev->cpu. [ ... ] > +static long cla_mdev_ioctl(struct vfio_device *vdev, unsigned int cmd, > + unsigned long arg) > +{ > + struct cla_mdev *mdev = > + container_of(vdev, struct cla_mdev, vdev); > + unsigned long minsz; > + > + switch (cmd) { > + case VFIO_DEVICE_GET_INFO: > + { [ ... ] > + } > + case VFIO_DEVICE_RESET: > + return cla_mdev_reset(mdev); > + case VFIO_DEVICE_GET_IRQ_INFO: > + case VFIO_DEVICE_SET_IRQS: > + default: > + return -EINVAL; > + } > + return -ENOTTY; [Severity: Low] Does this ioctl handler incorrectly return -EINVAL instead of -ENOTTY for unknown commands? The default case catches all unknown ioctls and returns -EINVAL, which makes the final return -ENOTTY unreachable. Userspace applications probing for features often rely on -ENOTTY to properly detect unsupported commands. Returning -EINVAL could mislead them into thinking the command is supported but their arguments are invalid. -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=15
