Re: [PATCH v2 11/19] driver core: Introduce a generic device trust level

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [High] driver core: device_trust_prepare_full() breaks default boots
  with -EPERM
- [Medium] driver core: device_trust_prepare_full() calls
  trust_resolve() unlocked
--

commit cc9d85465ea19f97fc7a0889f6993f37d1a58048
Author: Leon Romanovsky <leon@kernel.org>

driver core: Introduce a generic device trust level

This commit adds generic device trust admission and a device_trust.default
boot parameter to resolve a device's trust policy into an active trust
level before driver probing.

> diff --git a/drivers/base/trust.c b/drivers/base/trust.c
> new file mode 100644
> index 0000000000000..6a71abf10b3bb
> --- /dev/null
> +++ b/drivers/base/trust.c
> @@ -0,0 +1,132 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +#include <linux/device.h>
> +#include <linux/errno.h>
> +#include <linux/export.h>
> +#include <linux/init.h>
> +#include <linux/string.h>
> +#include <kunit/visibility.h>
> +
> +#include "base.h"
> +
> +static enum device_trust_policy device_trust_default_policy __ro_after_init;

[ ... ]

> +static int __device_trust_prepare(struct device *dev,
> +				  const struct device_driver *drv, bool full_only)
> +{
> +	enum device_trust_policy policy;
> +	enum device_trust_level level;
> +	int ret;
> +
> +	policy = dev->trust_policy;
> +
> +	if (policy == DEVICE_TRUST_POLICY_DISABLED)
> +		return -EPERM;
> +	if (dev->bus && dev->bus->trust_resolve) {
> +		ret = dev->bus->trust_resolve(dev, drv, policy, &level);
> +		if (ret)
> +			return ret;
> +	} else {
> +		level = policy == DEVICE_TRUST_POLICY_ADVERSARY ?
> +			DEVICE_TRUST_ADVERSARY : DEVICE_TRUST_FULL;
> +	}
> +	if (level == DEVICE_TRUST_DISABLED)
> +		return -EPERM;
> +	if (full_only && level != DEVICE_TRUST_FULL)
> +		return -EPERM;
> +	if (dev->p->trust_state == DEVICE_TRUST_COMMITTED &&
> +	    dev->trust_committed_level != level)
> +		return -EINVAL;
> +
> +	WRITE_ONCE(dev->trust_committed_level, level);
> +	dev->p->trust_state = DEVICE_TRUST_COMMITTED;
> +	WRITE_ONCE(dev->trust_level, level);
> +	return 0;
> +}
> +
> +int device_trust_prepare(struct device *dev, const struct device_driver *drv)
> +{
> +	device_lock_assert(dev);
> +	return __device_trust_prepare(dev, drv, false);
> +}
> +
> +int device_trust_prepare_full(struct device *dev, const struct device_driver *drv)
> +{
> +	if (device_trust_default_policy != DEVICE_TRUST_POLICY_FULL)
> +		return -EPERM;

[Severity: High]
Does requiring device_trust_default_policy == DEVICE_TRUST_POLICY_FULL
here cause device_driver_attach_full() to fail with -EPERM on every
default boot where device_trust.default= is not passed on the command
line?

When device_trust.default= is omitted, device_trust_default_policy remains
DEVICE_TRUST_POLICY_DEFAULT, even though that policy resolves to
DEVICE_TRUST_FULL on buses without trust_resolve.  Callers converted to
device_driver_attach_full(), such as phy_attach_direct() when attaching
the generic PHY fallback driver and serio_bind_driver(), will then fail
with -EPERM unless the kernel is booted with device_trust.default=full.

> +
> +	return __device_trust_prepare(dev, drv, true);
> +}

[Severity: Medium]
Can device_trust_prepare_full() end up calling dev->bus->trust_resolve()
without holding the device lock?

The kernel-doc for struct bus_type::trust_resolve requires the device lock
to be held, and the kernel-doc for device_driver_attach_full() states that
buses with a trust resolver must use device_driver_attach().  Unlike
device_bind_driver(), which rejects dev->bus->trust_resolve with
-EOPNOTSUPP, device_trust_prepare_full() calls __device_trust_prepare()
unlocked without checking dev->bus->trust_resolve.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261109-device-trust-core-v2-0-3b12d9a9f861@nvidia.com?part=11