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 <[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 consumers such as OP-TEE can use 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..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/[email protected]?part=2
