Re: [PATCH v4 1/3] rpmb: core: Guard frame requests and teardown with mutex

[Bean Huo]

On Sun, 2026-09-13 at 11:36 +0800, Stanley Jhu wrote:
> rpmb_route_frames() has no serialisation. That has two independent
> consequences.
> 
> Concurrent requests on one rpmb_dev corrupt each other. Two kthreads on
> different CPUs issuing RPMB_GET_WRITE_COUNTER against the same UFS RPMB
> region, 20000 iterations each:
> 
>   thread 0 done: 20000 iterations, mismatches=10889, errors=0
>   thread 1 done: 20000 iterations, mismatches=6141, errors=0
>   race complete: MISMATCH=17030 ERRORS=0
> 
> A mismatch is a response whose echoed nonce belongs to the other thread,
> so 43% of requests returned somebody else's frame. The errors count is
> transport failures, so nothing failed and the corruption is silent to
> the caller. An authenticated RPMB operation is not one command. The UFS
> provider issues SECURITY PROTOCOL OUT carrying the request, then
> SECURITY PROTOCOL IN to collect the response, with a result read request
> in between for write-type operations. JESD220F 12.4.7 states that any
> request other than a result read overwrites the result register of the
> region, so a request landing in the middle of another initiator's
> sequence destroys its response. The same chapter states that a region
> processes one authenticated operation at a time, so the rpmb_dev is the
> granularity the device itself assumes.
> 
> A request can also still be in flight when the provider tears down. The
> core reaches the provider through rdev->dev.parent, and the provider is
> free to release that device as soon as rpmb_dev_unregister() returns.
> Reference counting on the rpmb_dev does not prevent this:
> rpmb_dev_unregister() calls device_del(), which drops the reference that
> device_add() took on the parent, so the parent can be freed while the
> rpmb_dev is still alive and still routable. Unbinding a UFS host while a
> consumer holds an rpmb_dev reference and keeps issuing requests:
> 
>   BUG: KASAN: slab-use-after-free in ufs_rpmb_route_frames+0x328/0x420
>   Read of size 8 at addr fff00000c833bce8 by task rpmb_hold/100
>   Call trace:
>    ufs_rpmb_route_frames+0x328/0x420
>    rpmb_route_frames+0x64/0xd0
>   Freed by task 1:
>    kfree+0x2b8/0x5c4
>    ufs_rpmb_device_release+0x3c/0x60
>    device_release+0xa0/0x1fc
>    device_unregister+0x20/0x38
>    ufs_rpmb_remove+0x130/0x230
>    ufshcd_remove+0x54/0x22c
> 
> Both measurements needed patches 2/3 and 3/3 of this series applied,
> because UFS RPMB registration fails on mainline. The stable tag is for
> eMMC, which registers today. mmc_route_rpmb_frames() packs the whole
> sequence into one block request, so eMMC is not exposed to the
> interleaving above, but it does have the teardown window:
> mmc_blk_remove() reaches rpmb_dev_unregister() through
> mmc_blk_remove_parts() near its start,while the queue that
> mmc_route_rpmb_frames() submits to is only torn down at the end by
> mmc_blk_remove_req(), whose comment notes that it is freeing the queue
> that stops new requests being accepted. The eMMC window is from source
> reading; I have not reproduced it.
> 
> The kerneldoc change is part of the fix. Calling rpmb_dev_unregister()
> from a release callback cannot work, because the child rpmb_dev holds a
> reference on its parent, so the parent's release callback never runs.
> The UFS provider does exactly that today, inert only because its
> registration fails; patch 2/3 moves the call to the remove path.
> 
> Add a mutex and a dead flag to struct rpmb_dev. rpmb_route_frames()
> holds the mutex across the whole sequence and returns -ENODEV once the
> flag is set. rpmb_dev_unregister() sets the flag under the mutex before
> device_del(), so it cannot return while a request is inside the
> provider. Closing the teardown window requires excluding unregistration
> for the whole duration of a request, which is the same exclusion that
> serialises two requests, so one mutex covers both.
> 
> On the same test with this patch applied, MISMATCH=0 and the unbind is
> clean.
> 

Hi Stanley,

The fix looks right to me, the commit message is too long, it is easier to read
the code changes than the message.


> Fixes: 1e9046e3a154 ("rpmb: add Replay Protected Memory Block (RPMB)
> subsystem")
> Cc: stable@xxxxxxxxxxxxxxx
> Signed-off-by: Stanley Jhu <stanleyjhu@xxxxxxxxxx>
> ---
>  drivers/misc/rpmb-core.c | 34 +++++++++++++++++++++++++++++-----
>  include/linux/rpmb.h     |  6 ++++++
>  2 files changed, 35 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/misc/rpmb-core.c b/drivers/misc/rpmb-core.c
> index ecf14acf230a..bbc3c404ad6f 100644
> --- a/drivers/misc/rpmb-core.c
> +++ b/drivers/misc/rpmb-core.c
> @@ -45,16 +45,28 @@ EXPORT_SYMBOL_GPL(rpmb_dev_put);
>   * @rsp:       rpmb response frames
>   * @rsp_len:   length of rpmb response frames in bytes
>   *
> + * Context: Might sleep.
> + *
>   * Returns: < 0 on failure
>   */
>  int rpmb_route_frames(struct rpmb_dev *rdev, u8 *req,
>                       unsigned int req_len, u8 *rsp, unsigned int rsp_len)
>  {
> -       if (!req || !req_len || !rsp || !rsp_len)
> +       int ret;
> +
> +       if (!rdev || !req || !req_len || !rsp || !rsp_len)
>                 return -EINVAL;
>  
> -       return rdev->descr.route_frames(rdev->dev.parent, req, req_len,
> -                                       rsp, rsp_len);
> +       mutex_lock(&rdev->lock);
> +       if (rdev->dead) {
> +               I (&rdev->lock);
> +               return -ENODEV;
> +       }
> +
> +       ret = rdev->descr.route_frames(rdev->dev.parent, req, req_len,
> +                                      rsp, rsp_len);
> +       mutex_unlock(&rdev->lock);
> +       return ret;
>  }
>  EXPORT_SYMBOL_GPL(rpmb_route_frames);
>  
> @@ -62,6 +74,7 @@ static void rpmb_dev_release(struct device *dev)
>  {
>         struct rpmb_dev *rdev = to_rpmb_dev(dev);
>  
> +       mutex_destroy(&rdev->lock);
>         ida_free(&rpmb_ida, rdev->id);
>         kfree(rdev->descr.dev_id);
>         kfree(rdev);
> @@ -123,8 +136,9 @@ EXPORT_SYMBOL_GPL(rpmb_interface_unregister);
>   * rpmb_dev_unregister() - unregister RPMB partition from the RPMB subsystem
>   * @rdev: the rpmb device to unregister
>   *
> - * This function should be called from the release function of the
> - * underlying device used when the RPMB device was registered.
> + * This function should be called from the remove or unbind callback of the
> + * underlying device used when the RPMB device was registered, never from
> + * a device release callback.
>   *
>   * Returns: < 0 on failure
>   */
> @@ -133,6 +147,14 @@ int rpmb_dev_unregister(struct rpmb_dev *rdev)
>         if (!rdev)
>                 return -EINVAL;
>  
> +       mutex_lock(&rdev->lock);
> +       if (rdev->dead) {
> +               mutex_unlock(&rdev->lock);
> +               return 0;
> +       }
> +       rdev->dead = true;
> +       mutex_unlock(&rdev->lock);
> +
>         device_del(&rdev->dev);
>  
>         rpmb_dev_put(rdev);
> @@ -164,6 +186,7 @@ struct rpmb_dev *rpmb_dev_register(struct device *dev,
>         rdev = kzalloc_obj(*rdev);
>         if (!rdev)
>                 return ERR_PTR(-ENOMEM);
> +       mutex_init(&rdev->lock);
>         rdev->descr = *descr;
>         rdev->descr.dev_id = kmemdup(descr->dev_id, descr->dev_id_len,
>                                      GFP_KERNEL);
> @@ -194,6 +217,7 @@ struct rpmb_dev *rpmb_dev_register(struct device *dev,
>  err_free_dev_id:
>         kfree(rdev->descr.dev_id);
>  err_free_rdev:
> +       mutex_destroy(&rdev->lock);
>         kfree(rdev);
>         return ERR_PTR(ret);
>  }
> diff --git a/include/linux/rpmb.h b/include/linux/rpmb.h
> index ed3f8e431eff..814ac3e69337 100644
> --- a/include/linux/rpmb.h
> +++ b/include/linux/rpmb.h
> @@ -7,6 +7,7 @@
>  #define __RPMB_H__
>  
>  #include <linux/device.h>
> +#include <linux/mutex.h>
>  #include <linux/types.h>
>  
>  /**
> @@ -48,15 +49,20 @@ struct rpmb_descr {
>   * struct rpmb_dev - device which can support RPMB partition
>   *
>   * @dev              : device
> + * @lock             : protects in-flight operations against teardown
>   * @id               : device_id
>   * @list_node        : linked list node
>   * @descr            : RPMB description
> + * @dead             : set to true when device is unregistered
>   */
>  struct rpmb_dev {
>         struct device dev;
> +       /* Protects in-flight operations against teardown */
> +       struct mutex lock;

This lock also makes route_frames calls run one at a time, not only protect
against teardown.


>         int id;
>         struct list_head list_node;
>         struct rpmb_descr descr;
> +       bool dead;
>  };
>  
>  #define to_rpmb_dev(x)         container_of((x), struct rpmb_dev, dev)

Both are nits, feel free to add:

Reviewed-by: Bean Huo <beanhuo@xxxxxxxxxx>

Kind regards,
Bean