Re: [PATCH v4] mmc: core: Use mrq.sbc in close-ended ffu

[Mikko Rapeli <mikko.rapeli@xxxxxxxxxx>]

On Wed, Mar 13, 2024 at 01:46:44PM +0200, Mikko Rapeli wrote:
<snip>
> Fix will be to initialize mmc_blk_ioc_data->flags in all cases. Would this be fine as
> a catch all initialization for mmc_blk_ioc_data?
> 
> --- a/drivers/mmc/core/block.c
> +++ b/drivers/mmc/core/block.c
> @@ -413,7 +413,7 @@ static struct mmc_blk_ioc_data *mmc_blk_ioctl_copy_from_user(
>         struct mmc_blk_ioc_data *idata;
>         int err;
>  
> -       idata = kmalloc(sizeof(*idata), GFP_KERNEL);
> +       idata = kzalloc(sizeof(*idata), GFP_KERNEL);
>         if (!idata) {
>                 err = -ENOMEM;
>                 goto out;
> 
> I think this is a sensible and future proof way to go.
> 
> Then, the second flags check for MMC_BLK_IOC_SBC is accessing array using
> index i - 1, but is not checking if i == 0 which results in data[-1] access.
> I think this should be fixed with something like:
> 
> -       if (idata->flags & MMC_BLK_IOC_SBC)
> +       if (idata->flags & MMC_BLK_IOC_SBC && i > 0) {
> 
> Would you be fine with this?

In addition to these, is the original patch correct also when idata->rpmb is not true
but prev_idata is? I have no idea of the code but this looks a bit suspicious.

@@ -532,7 +544,7 @@ static int __mmc_blk_ioctl_cmd(struct mmc_card *card, struct mmc_blk_data *md,
                        return err;
        }
 
-       if (idata->rpmb) {
+       if (idata->rpmb || prev_idata) {
                sbc.opcode = MMC_SET_BLOCK_COUNT;
                /*
                 * We don't do any blockcount validation because the max size
@@ -540,6 +552,8 @@ static int __mmc_blk_ioctl_cmd(struct mmc_card *card, struct mmc_blk_data *md,
                 * 'Reliable Write' bit here.
                 */
                sbc.arg = data.blocks | (idata->ic.write_flag & BIT(31));
+               if (prev_idata)
+                       sbc.arg = prev_idata->ic.arg;
                sbc.flags = MMC_RSP_R1 | MMC_CMD_AC;
                mrq.sbc = &sbc;
        }

Cheers,

-Mikko