Re: [RFC PATCH v2 15/15] misc/arm-cla: Add vfio-mdev user interface

[sashiko-bot@kernel.org]

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 <jpb@kernel.org>

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/20261015142552.4128028-1-ryan.roberts@arm.com?part=15