Re: [PATCH 2/2] rpmb: add a virtio RPMB device driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] rpmb: virtio_rpmb_probe() erroneously divides max_wr by 2
- [Medium] virtio_rpmb: vr->vq published in virtio_rpmb_init_vq() before device ready
--

commit 0fd82617fed90856fa2c50ebdd578db85ac1bddd
Author: Haruka Kuze <harukak@zohomail.eu>

rpmb: add a virtio RPMB device driver

This commit adds a driver for virtio-rpmb devices (virtio ID 28) and registers
them with the RPMB subsystem as RPMB_TYPE_EMMC so in-kernel consumers such as
OP-TEE can use them.

Link: https://lore.kernel.org/all/20220405093759.1126835-4-alex.bennee@linaro.org/

> diff --git a/drivers/misc/virtio_rpmb.c b/drivers/misc/virtio_rpmb.c
> new file mode 100644
> index 0000000000000..f02bdf13aa43c
> --- /dev/null
> +++ b/drivers/misc/virtio_rpmb.c

[ ... ]

> +static int virtio_rpmb_init_vq(struct virtio_rpmb *vr)
> +{
> +	struct virtqueue *vq;
> +
> +	vq = virtio_find_single_vq(vr->vdev, virtio_rpmb_done, "request");
> +	if (IS_ERR(vq))
> +		return PTR_ERR(vq);
> +
> +	mutex_lock(&vr->lock);
> +	vr->vq = vq;
> +	mutex_unlock(&vr->lock);
> +	return 0;
> +}

[ ... ]

> +static int virtio_rpmb_probe(struct virtio_device *vdev)
> +{
> +	struct rpmb_descr descr = {
> +		/* Frames and their semantics are that of eMMC */
> +		.type = RPMB_TYPE_EMMC,
> +		.route_frames = virtio_rpmb_route_frames,
> +		.dev_id = (u8 *)virtio_rpmb_dev_id,
> +		.dev_id_len = sizeof(virtio_rpmb_dev_id),
> +	};
> +	struct virtio_rpmb *vr;
> +	u8 capacity;
> +	int ret;
> +
> +	capacity = virtio_cread8(vdev, offsetof(struct virtio_rpmb_config,
> +						capacity));
> +	if (!capacity || capacity > 0x80)
> +		return dev_err_probe(&vdev->dev, -EINVAL,
> +				     "invalid capacity %u\n", capacity);
> +
> +	vr = devm_kzalloc(&vdev->dev, sizeof(*vr), GFP_KERNEL);
> +	if (!vr)
> +		return -ENOMEM;
> +
> +	vdev->priv = vr;
> +	vr->vdev = vdev;
> +	mutex_init(&vr->lock);
> +	vr->max_wr = virtio_cread8(vdev, offsetof(struct virtio_rpmb_config,
> +						  max_wr_cnt));
> +	vr->max_rd = virtio_cread8(vdev, offsetof(struct virtio_rpmb_config,
> +						  max_rd_cnt));
> +
> +	ret = virtio_rpmb_init_vq(vr);
> +	if (ret)
> +		return ret;
> +
> +	virtio_device_ready(vdev);
> +
> +	/*
> +	 * eMMC reports the reliable wr count in 512 byte sectors, but the device
> +	 * limit is in 256 byte blocks.
> +	 */
> +	descr.capacity = capacity;
> +	descr.reliable_wr_count = (vr->max_wr ?: U8_MAX) / 2;

[Severity: High]
Should descr.reliable_wr_count be set without dividing vr->max_wr by 2?

Each 256-byte RPMB data block is carried inside a single 512-byte
struct virtio_rpmb_frame, so one 256-byte RPMB block corresponds 1-to-1 with
one 512-byte RPMB frame/sector. This matches how virtio_rpmb_route_frames()
validates write requests against vr->max_wr:

	case VIRTIO_RPMB_REQ_DATA_WRITE:
		if (req_len % VIRTIO_RPMB_FRAME_SIZE ||
		    resp_len != VIRTIO_RPMB_FRAME_SIZE)
			return -EINVAL;
		if (vr->max_wr && req_len / VIRTIO_RPMB_FRAME_SIZE > vr->max_wr)
			return -EINVAL;

When a virtio-rpmb device reports max_wr_cnt == 1, dividing vr->max_wr by 2
truncates descr.reliable_wr_count to 0. In-kernel consumers such as OP-TEE
receive rdev->descr.reliable_wr_count directly in
handle_rpc_func_rpmb_probe_next(), so reporting 0 makes the device unusable
for writes when max_wr_cnt is 1, and halves the maximum write count for larger
values.

> +
> +	vr->rdev = rpmb_dev_register(&vdev->dev, &descr);
> +	if (IS_ERR(vr->rdev)) {
> +		virtio_rpmb_del_vq(vr);
> +		return PTR_ERR(vr->rdev);
> +	}
> +
> +	return 0;
> +}
> +
> +static void virtio_rpmb_remove(struct virtio_device *vdev)
> +{
> +	struct virtio_rpmb *vr = vdev->priv;
> +
> +	rpmb_dev_unregister(vr->rdev);
> +	virtio_rpmb_del_vq(vr);
> +}
> +
> +static int virtio_rpmb_freeze(struct virtio_device *vdev)
> +{
> +	virtio_rpmb_del_vq(vdev->priv);
> +	return 0;
> +}
> +
> +static int virtio_rpmb_restore(struct virtio_device *vdev)
> +{
> +	return virtio_rpmb_init_vq(vdev->priv);
> +}

[Severity: Medium]
Can virtio_rpmb_restore() allow virtio_rpmb_xfer() to kick the virtqueue
before the device is marked VIRTIO_CONFIG_S_DRIVER_OK?

During freeze and restore, vr->rdev remains registered with the RPMB core, and
virtio_rpmb_xfer() uses vr->vq under vr->lock to check whether the device is
available:

	mutex_lock(&vr->lock);
	if (!vr->vq) {
		ret = -ENODEV;
		goto out;
	}

	ret = virtqueue_add_sgs(vr->vq, sgs, 1, 1, &req, GFP_KERNEL);
	if (ret)
		goto out;

	virtqueue_kick(vr->vq);
	wait_for_completion(&req.done);

In drivers/virtio/virtio.c, virtio_device_restore_priv() only calls
virtio_device_ready(dev) after drv->restore(dev) returns:

	if (restore) {
		if (drv->restore) {
			ret = drv->restore(dev);
			if (ret)
				goto err;
		}
	...
	/* If restore didn't do it, mark device DRIVER_OK ourselves. */
	if (!(dev->config->get_status(dev) & VIRTIO_CONFIG_S_DRIVER_OK))
		virtio_device_ready(dev);

Because virtio_rpmb_init_vq() sets vr->vq = vq and releases vr->lock before
virtio_device_ready(vdev) is called, a concurrent rpmb_route_frames() ->
virtio_rpmb_route_frames() -> virtio_rpmb_xfer() call can observe a non-NULL
vr->vq and call virtqueue_add_sgs() and virtqueue_kick() before
VIRTIO_CONFIG_S_DRIVER_OK is set (or while vq->broken is still true under
CONFIG_VIRTIO_HARDEN_NOTIFICATION).

Should virtio_device_ready(vr->vdev) be called inside virtio_rpmb_init_vq()
before publishing vr->vq under vr->lock?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261029081317.2264887-1-harukak@zohomail.eu?part=2