Re: [PATCH v2 2/2] ufs: rpmb: use a fixed-length RPMB dev_id

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

Pre-existing issues:
- [High] Use-After-Free on device unbind due to devm_kzalloc used for struct embedding a device
- [High] Circular dependency between parent and child devices prevents cleanup and causes resource leak
- [High] NULL pointer dereference in ufs_rpmb_remove if ufs_rpmb_probe bails out early
--

--- Patch [2]: [PATCH v2 2/2] ufs: rpmb: use a fixed-length RPMB dev_id ---
commit 2720876781cfffc7b6138bf9c11da0d2249234f9
Author: Jorge Ramirez-Ortiz <jorge.ramirez@oss.qualcomm.com>

ufs: rpmb: use a fixed-length RPMB dev_id

The RPMB authentication key is derived from the dev_id handed to the
RPMB subsystem. This commit hashes the variable-length UFS id into a fixed
16-byte dev_id using blake2b to match the eMMC CID layout OP-TEE relies on.

> diff --git a/drivers/ufs/core/ufs-rpmb.c b/drivers/ufs/core/ufs-rpmb.c
> index d0c7ea7a36f49..0d8399b6497a1 100644
> --- a/drivers/ufs/core/ufs-rpmb.c
> +++ b/drivers/ufs/core/ufs-rpmb.c

[ ... ]

> @@ -154,6 +156,7 @@ int ufs_rpmb_probe(struct ufs_hba *hba)
>  {
>  	struct ufs_rpmb_dev *ufs_rpmb, *it, *tmp;
>  	struct rpmb_dev *rdev;
> +	char *dev_id = NULL;
>  	char *cid = NULL;

[Severity: High]
This isn't a bug introduced by this patch, but does the early return in
ufs_rpmb_probe() leave the rpmbs list head uninitialized?

If the function returns early due to a missing device ID:

drivers/ufs/core/ufs-rpmb.c:ufs_rpmb_probe() {
    if (!hba->dev_info.device_id) {
        dev_err(hba->dev, "UFS Device ID not available\n");
        return -EINVAL;
    }

    INIT_LIST_HEAD(&hba->rpmbs);
    ...
}

When ufs_rpmb_remove() is later called on driver unbind, list_empty()
evaluates to false because hba->rpmbs is completely zeroed out rather than
pointing to itself. This causes a NULL pointer dereference when traversing
the list:

drivers/ufs/core/ufs-rpmb.c:ufs_rpmb_remove() {
    if (list_empty(&hba->rpmbs))
        return;

    list_for_each_entry_safe(ufs_rpmb, tmp, &hba->rpmbs, node) {
    ...
}

[Severity: High]
This is a pre-existing issue, but does using devm_kzalloc() for a structure
that embeds a device cause a use-after-free on driver unbind?

drivers/ufs/core/ufs-rpmb.c:ufs_rpmb_probe() {
    ...
    ufs_rpmb = devm_kzalloc(hba->dev, sizeof(*ufs_rpmb), GFP_KERNEL);
    ...
    device_register(&ufs_rpmb->dev);
    ...
}

If a userspace process opens a sysfs file for this device, the device
reference count is elevated. If the UFS driver is unbound at this point,
devres will automatically free the ufs_rpmb structure.

Later, when the sysfs file is closed, the device core drops the final
reference and calls ufs_rpmb_device_release(), which accesses the
already-freed memory:

drivers/ufs/core/ufs-rpmb.c:ufs_rpmb_device_release() {
    struct ufs_rpmb_dev *ufs_rpmb = dev_get_drvdata(dev);

    rpmb_dev_unregister(ufs_rpmb->rdev);
}

Should ufs_rpmb be allocated with a standard kzalloc() and explicitly freed
in the device release callback instead?

[Severity: High]
This is a pre-existing issue, but is there a circular dependency preventing
the release callback from ever running?

The child device (rdev) holds a reference to its parent (ufs_rpmb->dev).
However, the child is only unregistered in the parent's release callback:

drivers/ufs/core/ufs-rpmb.c:ufs_rpmb_device_release() {
    struct ufs_rpmb_dev *ufs_rpmb = dev_get_drvdata(dev);

    rpmb_dev_unregister(ufs_rpmb->rdev);
}

During driver unbind, only the parent device is unregistered:

drivers/ufs/core/ufs-rpmb.c:ufs_rpmb_remove() {
    ...
    list_for_each_entry_safe(ufs_rpmb, tmp, &hba->rpmbs, node) {
        ...
        device_unregister(&ufs_rpmb->dev);
    }
}

Because the child is left registered, it continues to hold a reference to
the parent. As a result, the parent's reference count never reaches zero,
ufs_rpmb_device_release() is never called, and the device structures are
leaked.

Should the child device be explicitly unregistered in ufs_rpmb_remove()
before unregistering the parent?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260720091614.544968-1-jorge.ramirez@oss.qualcomm.com?part=2