Enabling RPMB overwrites setting the boot partition via ioctl

Enabling RPMB overwrites setting the boot partition via ioctl

[Schaefer, Brandon]

Linux kernel: 4.1.15 (imx_4.1.15_1.0.0_ga branch)

I've noticed that writing to the boot partition is overwritten by RPMB related functions as it stores this byte (card->ext_csd.part_conf). It is necessary to update this reference when issuing CMD6 SWITCH through ioctl. See below patch for above kernel version:

diff --git a/drivers/mmc/card/block.c b/drivers/mmc/card/block.c
index be776fa..cbd0533 100644
--- a/drivers/mmc/card/block.c
+++ b/drivers/mmc/card/block.c
@@ -60,7 +60,9 @@ MODULE_ALIAS("mmc:block");
#define INAND_CMD38_ARG_SECTRIM2 0x88
#define MMC_BLK_TIMEOUT_MS  (10 * 60 * 1000)        /* 10 minute timeout */
#define MMC_SANITIZE_REQ_TIMEOUT 240000
-#define MMC_EXTRACT_INDEX_FROM_ARG(x) ((x & 0x00FF0000) >> 16)
+#define MMC_EXTRACT_SWITCH_MODE_FROM_ARG(x)  (((x) & 0x03000000) >> 24)
+#define MMC_EXTRACT_INDEX_FROM_ARG(x)        (((x) & 0x00FF0000) >> 16)
+#define MMC_EXTRACT_VALUE_FROM_ARG(x)        (((x) & 0x0000FF00) >> 8)

 #define mmc_req_rel_wr(req)   (((req->cmd_flags & REQ_FUA) || \
                                (req->cmd_flags & REQ_META)) && \
@@ -572,6 +574,24 @@ static int mmc_blk_ioctl_cmd(struct block_device *bdev,
              goto cmd_rel_host;
       }

+       /* If writing part config register, update struct */
+       if ((MMC_EXTRACT_INDEX_FROM_ARG(cmd.arg) == EXT_CSD_PART_CONFIG) &&
+           (cmd.opcode == MMC_SWITCH)) {
+              u8 mode = MMC_EXTRACT_SWITCH_MODE_FROM_ARG(cmd.arg);
+              u8 value = MMC_EXTRACT_VALUE_FROM_ARG(cmd.arg);
+              switch (mode) {
+              case MMC_SWITCH_MODE_SET_BITS:
+                      card->ext_csd.part_config |= value;
+                      break;
+              case MMC_SWITCH_MODE_CLEAR_BITS:
+                      card->ext_csd.part_config &= ~value;
+                      break;
+              case MMC_SWITCH_MODE_WRITE_BYTE:
+                      card->ext_csd.part_config = value;
+                      break;
+              }
+       }
+
       /*
        * According to the SD specs, some commands require a delay after
        * issuing the command.
________________________________
________________________________


Please be advised that this email may contain confidential information. If you are not the intended recipient, please notify us by email by replying to the sender and delete this message. The sender disclaims that the content of this email constitutes an offer to enter into, or the acceptance of, any agreement; provided that the foregoing does not invalidate the binding effect of any digital or other electronic reproduction of a manual signature that is included in any attachment.

Re: Enabling RPMB overwrites setting the boot partition via ioctl

[Ulf Hansson]

+ Bean Huo, Shawn Lin, Bartlomiej Zolnierkiewicz,

On 20 May 2017 at 00:03, Schaefer, Brandon
<brandon.schaefer@flukenetworks.com> wrote:
> Linux kernel: 4.1.15 (imx_4.1.15_1.0.0_ga branch)
>
> I've noticed that writing to the boot partition is overwritten by RPMB related functions as it stores this byte (card->ext_csd.part_conf). It is necessary to update this reference when issuing CMD6 SWITCH through ioctl. See below patch for above kernel version:

Thanks for sharing the information!

>
> diff --git a/drivers/mmc/card/block.c b/drivers/mmc/card/block.c
> index be776fa..cbd0533 100644
> --- a/drivers/mmc/card/block.c
> +++ b/drivers/mmc/card/block.c
> @@ -60,7 +60,9 @@ MODULE_ALIAS("mmc:block");
> #define INAND_CMD38_ARG_SECTRIM2 0x88
> #define MMC_BLK_TIMEOUT_MS  (10 * 60 * 1000)        /* 10 minute timeout */
> #define MMC_SANITIZE_REQ_TIMEOUT 240000
> -#define MMC_EXTRACT_INDEX_FROM_ARG(x) ((x & 0x00FF0000) >> 16)
> +#define MMC_EXTRACT_SWITCH_MODE_FROM_ARG(x)  (((x) & 0x03000000) >> 24)
> +#define MMC_EXTRACT_INDEX_FROM_ARG(x)        (((x) & 0x00FF0000) >> 16)
> +#define MMC_EXTRACT_VALUE_FROM_ARG(x)        (((x) & 0x0000FF00) >> 8)
>
>  #define mmc_req_rel_wr(req)   (((req->cmd_flags & REQ_FUA) || \
>                                 (req->cmd_flags & REQ_META)) && \
> @@ -572,6 +574,24 @@ static int mmc_blk_ioctl_cmd(struct block_device *bdev,
>               goto cmd_rel_host;
>        }
>
> +       /* If writing part config register, update struct */
> +       if ((MMC_EXTRACT_INDEX_FROM_ARG(cmd.arg) == EXT_CSD_PART_CONFIG) &&
> +           (cmd.opcode == MMC_SWITCH)) {
> +              u8 mode = MMC_EXTRACT_SWITCH_MODE_FROM_ARG(cmd.arg);
> +              u8 value = MMC_EXTRACT_VALUE_FROM_ARG(cmd.arg);
> +              switch (mode) {
> +              case MMC_SWITCH_MODE_SET_BITS:
> +                      card->ext_csd.part_config |= value;
> +                      break;
> +              case MMC_SWITCH_MODE_CLEAR_BITS:
> +                      card->ext_csd.part_config &= ~value;
> +                      break;
> +              case MMC_SWITCH_MODE_WRITE_BYTE:
> +                      card->ext_csd.part_config = value;
> +                      break;
> +              }
> +       }
> +
>        /*
>         * According to the SD specs, some commands require a delay after
>         * issuing the command.

I am not sure how to go with this, especially since you are referring
to an old kernel, 4.1.15. Can you confirm that the problem is still
present in the my next branch of my mmc git tree? That would be more
relevant information to start with.

So to move forward, I decide to investigate the problem a bit. Here's
my summary:

1) There are problems regarding switching between partitions in
general, because the (e)MMC partitions isn't correctly set up.
2) The are problems related to when userspace utilizes the mmc
back-door (mmc-ioctl) to change ext csd, because the mmc core don't
get informed about these updates. For example, the mmc core may then
take wrong decisions in the suspend/resume sequence.

It would really be nice to fix these problems, however I need good
quality patches, based on my mmc next branch confirming to fix
problems.

Here are some interesting samples from my investigation.
https://patchwork.kernel.org/patch/9632365/
https://community.nxp.com/thread/340088
http://www.spinics.net/lists/linux-mmc/msg13516.html


And here is an interesting commit, which really puzzles me as issuing
a CMD0 should always re-set the eMMC device to the user partition. I
don't get why this is needed and nor how it can fix a problem.

commit 41e2a4893566ced3c46af15df5b727326881e47d
Author: Philip Rakity <prakity@marvell.com>
Date:   Sat Mar 19 14:10:33 2011 -0400

    mmc: Ensure linux starts in eMMC user partition

    uBoot sometimes leaves eMMC pointing to the private boot partition.
    Ensure we always start looking at the user partition.


Kind regards
Uffe

Re: Enabling RPMB overwrites setting the boot partition via ioctl

[Shawn Lin]

Hi

On 2017/5/23 18:17, Ulf Hansson wrote:
> + Bean Huo, Shawn Lin, Bartlomiej Zolnierkiewicz,
>
> On 20 May 2017 at 00:03, Schaefer, Brandon
> <brandon.schaefer@flukenetworks.com> wrote:
>> Linux kernel: 4.1.15 (imx_4.1.15_1.0.0_ga branch)
>>
>> I've noticed that writing to the boot partition is overwritten by RPMB related functions as it stores this byte (card->ext_csd.part_conf). It is necessary to update this reference when issuing CMD6 SWITCH through ioctl. See below patch for above kernel version:
>
> Thanks for sharing the information!
>
>>
>> diff --git a/drivers/mmc/card/block.c b/drivers/mmc/card/block.c
>> index be776fa..cbd0533 100644
>> --- a/drivers/mmc/card/block.c
>> +++ b/drivers/mmc/card/block.c
>> @@ -60,7 +60,9 @@ MODULE_ALIAS("mmc:block");
>> #define INAND_CMD38_ARG_SECTRIM2 0x88
>> #define MMC_BLK_TIMEOUT_MS  (10 * 60 * 1000)        /* 10 minute timeout */
>> #define MMC_SANITIZE_REQ_TIMEOUT 240000
>> -#define MMC_EXTRACT_INDEX_FROM_ARG(x) ((x & 0x00FF0000) >> 16)
>> +#define MMC_EXTRACT_SWITCH_MODE_FROM_ARG(x)  (((x) & 0x03000000) >> 24)
>> +#define MMC_EXTRACT_INDEX_FROM_ARG(x)        (((x) & 0x00FF0000) >> 16)
>> +#define MMC_EXTRACT_VALUE_FROM_ARG(x)        (((x) & 0x0000FF00) >> 8)
>>
>>  #define mmc_req_rel_wr(req)   (((req->cmd_flags & REQ_FUA) || \
>>                                 (req->cmd_flags & REQ_META)) && \
>> @@ -572,6 +574,24 @@ static int mmc_blk_ioctl_cmd(struct block_device *bdev,
>>               goto cmd_rel_host;
>>        }
>>
>> +       /* If writing part config register, update struct */
>> +       if ((MMC_EXTRACT_INDEX_FROM_ARG(cmd.arg) == EXT_CSD_PART_CONFIG) &&
>> +           (cmd.opcode == MMC_SWITCH)) {
>> +              u8 mode = MMC_EXTRACT_SWITCH_MODE_FROM_ARG(cmd.arg);
>> +              u8 value = MMC_EXTRACT_VALUE_FROM_ARG(cmd.arg);
>> +              switch (mode) {
>> +              case MMC_SWITCH_MODE_SET_BITS:
>> +                      card->ext_csd.part_config |= value;
>> +                      break;
>> +              case MMC_SWITCH_MODE_CLEAR_BITS:
>> +                      card->ext_csd.part_config &= ~value;
>> +                      break;
>> +              case MMC_SWITCH_MODE_WRITE_BYTE:
>> +                      card->ext_csd.part_config = value;
>> +                      break;
>> +              }
>> +       }
>> +
>>        /*
>>         * According to the SD specs, some commands require a delay after
>>         * issuing the command.
>
> I am not sure how to go with this, especially since you are referring
> to an old kernel, 4.1.15. Can you confirm that the problem is still
> present in the my next branch of my mmc git tree? That would be more
> relevant information to start with.
>
> So to move forward, I decide to investigate the problem a bit. Here's
> my summary:
>
> 1) There are problems regarding switching between partitions in
> general, because the (e)MMC partitions isn't correctly set up.
> 2) The are problems related to when userspace utilizes the mmc
> back-door (mmc-ioctl) to change ext csd, because the mmc core don't
> get informed about these updates. For example, the mmc core may then
> take wrong decisions in the suspend/resume sequence.
>

patch[1] was also trying to fix one of these.
I think the core issue here is that we cached the ext_csd and provide
the ability for user space to r/w it without proper updating the whole
cached ext_csd. So could we just simply read ext_csd again if CMD6 was
issued by ioctl?


[1]: https://patchwork.kernel.org/patch/9632365/

> It would really be nice to fix these problems, however I need good
> quality patches, based on my mmc next branch confirming to fix
> problems.
>
> Here are some interesting samples from my investigation.
> https://patchwork.kernel.org/patch/9632365/
> https://community.nxp.com/thread/340088
> http://www.spinics.net/lists/linux-mmc/msg13516.html
>
>
> And here is an interesting commit, which really puzzles me as issuing
> a CMD0 should always re-set the eMMC device to the user partition. I
> don't get why this is needed and nor how it can fix a problem.
>
> commit 41e2a4893566ced3c46af15df5b727326881e47d
> Author: Philip Rakity <prakity@marvell.com>
> Date:   Sat Mar 19 14:10:33 2011 -0400
>
>     mmc: Ensure linux starts in eMMC user partition
>
>     uBoot sometimes leaves eMMC pointing to the private boot partition.
>     Ensure we always start looking at the user partition.
>
>
> Kind regards
> Uffe
> --
> To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
>
>

RE: Enabling RPMB overwrites setting the boot partition via ioctl

[Schaefer, Brandon]

Hello Uffe,

I most likely will not be able to confirm that the problem is still present in mmc-next for you, I do not have hardware available to do so. Looking at the code this bug does seem to be present as ext_csd.part_config is not updated in the ioctl, related to 2) in your summary. There could be other places similar to the scenario I've reported for other partsof ext csd, but I'm not familiar enough with either mmc specification or the code .

As for 1) cannot confirm as I've never witnessed this issue.

> I am not sure how to go with this, especially since you are referring to an old
> kernel, 4.1.15. Can you confirm that the problem is still present in the my
> next branch of my mmc git tree? That would be more relevant information to
> start with.
>
> So to move forward, I decide to investigate the problem a bit. Here's my
> summary:
>
> 1) There are problems regarding switching between partitions in general,
> because the (e)MMC partitions isn't correctly set up.
> 2) The are problems related to when userspace utilizes the mmc back-door
> (mmc-ioctl) to change ext csd, because the mmc core don't get informed
> about these updates. For example, the mmc core may then take wrong
> decisions in the suspend/resume sequence.
>
> It would really be nice to fix these problems, however I need good quality
> patches, based on my mmc next branch confirming to fix problems.
>

I can submit a patch for the bug I've witnessed if you'd like, which may end up being a series of similar fixes from others?

Brandon
________________________________
________________________________


Please be advised that this email may contain confidential information. If you are not the intended recipient, please notify us by email by replying to the sender and delete this message. The sender disclaims that the content of this email constitutes an offer to enter into, or the acceptance of, any agreement; provided that the foregoing does not invalidate the binding effect of any digital or other electronic reproduction of a manual signature that is included in any attachment.