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 <[email protected]> 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/[email protected]/ > 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/[email protected]?part=2
