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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] rpmb: virtio_rpmb: incorrect reliable_wr_count calculation in virtio_rpmb_probe()
- [High] rpmb: virtio_rpmb: race on virtqueue in virtio_rpmb_done()
- [Medium] virtio_rpmb: avoid duplicate del_vqs() in virtio_rpmb_del_vq() after failed restore
--

commit df2c2241a3933ce84e1976592ab925a107995087
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 users
such as OP-TEE can access 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..4caca378fecc4
> --- /dev/null
> +++ b/drivers/misc/virtio_rpmb.c

[ ... ]

> +static void virtio_rpmb_done(struct virtqueue *vq)
> +{
> +	struct virtio_rpmb_req *req;
> +	unsigned int len;
> +
> +	while ((req = virtqueue_get_buf(vq, &len))) {
> +		req->len = len;
> +		complete(&req->done);
> +	}
> +}
> +
> +static int virtio_rpmb_xfer(struct virtio_rpmb *vr, void *out,
> +			    unsigned int out_len, void *in, unsigned int in_len)
> +{
> +	struct scatterlist out_sg, in_sg, *sgs[2] = { &out_sg, &in_sg };
> +	struct virtio_rpmb_req req;
> +	int ret;
> +
> +	sg_init_one(&out_sg, out, out_len);
> +	sg_init_one(&in_sg, in, in_len);
> +	init_completion(&req.done);
> +
> +	guard(mutex)(&vr->lock);
> +	if (!vr->vq)
> +		return -ENODEV;
> +
> +	ret = virtqueue_add_sgs(vr->vq, sgs, 1, 1, &req, GFP_KERNEL);
> +	if (ret)
> +		return ret;
> +
> +	virtqueue_kick(vr->vq);
> +	wait_for_completion(&req.done);
> +
> +	return req.len == in_len ? 0 : -EIO;
> +}

[Severity: High]
Can virtio_rpmb_done() race with virtio_rpmb_xfer() on the virtqueue state?

In virtio_rpmb_done(), once complete(&req->done) is called on the first loop
iteration, the task waiting in virtio_rpmb_xfer() can wake up, exit the
function, and release vr->lock while virtio_rpmb_done() is still looping to
call virtqueue_get_buf(vq, &len) again:

CPU0                                    CPU1
virtio_rpmb_done()                      virtio_rpmb_xfer()
  req = virtqueue_get_buf(vq, &len);
  complete(&req->done);
                                          wait_for_completion(&req.done);
                                          /* releases vr->lock */
                                        virtio_rpmb_xfer()
                                          /* acquires vr->lock */
  virtqueue_get_buf(vq, &len);            virtqueue_add_sgs(vr->vq, ...);
                                          virtqueue_kick(vr->vq);

Additionally, because virtqueue_add_sgs() exposes the available descriptor
before returning, a polling host or spurious interrupt can trigger
virtio_rpmb_done() concurrently with virtqueue_add_sgs() or virtqueue_kick().

Since virtio_ring requires callers to synchronize virtqueue_get_buf() against
virtqueue_add_sgs() and virtqueue_kick(), could this corrupt virtqueue state
(such as vq->free_head, vq->vq.num_free, and vq->num_added) or trigger the
START_USE(vq) / END_USE(vq) checks in drivers/virtio/virtio_ring.c when DEBUG
is enabled?

[ ... ]

> +static void virtio_rpmb_del_vq(struct virtio_rpmb *vr)
> +{
> +	struct virtio_device *vdev = vr->vdev;
> +
> +	scoped_guard(mutex, &vr->lock)
> +		vr->vq = NULL;
> +
> +	virtio_reset_device(vdev);
> +	vdev->config->del_vqs(vdev);
> +}

[Severity: Medium]
What happens if virtio_rpmb_restore() fails after virtio_rpmb_freeze() has
already torn down the virtqueue?

During suspend, virtio_rpmb_freeze() calls virtio_rpmb_del_vq(), which sets
vr->vq = NULL, resets the device, and calls vdev->config->del_vqs(vdev).

If virtio_rpmb_restore() -> virtio_rpmb_init_vq() -> virtio_find_single_vq()
later fails during resume, vr->vq remains NULL while the driver stays bound
to the device. When virtio_rpmb_remove() or a subsequent virtio_rpmb_freeze()
runs, virtio_rpmb_del_vq() will unconditionally call
vdev->config->del_vqs(vdev) a second time without a matching find_vqs(),
which on transports such as virtio_mmio calls free_irq() on an already-freed
interrupt.

Should virtio_rpmb_del_vq() return early if vr->vq is already NULL?

> +
> +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;
> +
> +	/*
> +	 * 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 here?

Each 256-byte RPMB data block is carried inside a single 512-byte
virtio_rpmb_frame (one 512-byte eMMC sector per 256-byte RPMB data payload).
In virtio_rpmb_route_frames(), vr->max_wr is checked directly against the
number of 512-byte frames:

drivers/misc/virtio_rpmb.c:virtio_rpmb_route_frames() {
	...
	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;
	...
}

In the RPMB subsystem, descr.reliable_wr_count is also the number of RPMB
frames (256-byte data blocks) per write request, matching mmc_blk_rpmb_add()
in drivers/mmc/core/block.c:

drivers/mmc/core/block.c:mmc_blk_rpmb_add() {
	...
	struct rpmb_descr descr = {
		.type = RPMB_TYPE_EMMC,
		.route_frames = mmc_route_rpmb_frames,
		.reliable_wr_count = card->ext_csd.enhanced_rpmb_supported ?
				     2 : 32,
	...
}

Dividing by 2 in virtio_rpmb_probe() halves the advertised write count, and
when a device reports max_wr_cnt == 1, integer division truncates
descr.reliable_wr_count to 0.

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