[RFC,1/1] mmc-utils: Support-sending-eMMC-5.0-FFU

Commit Message

Avi Shchislowski Feb. 9, 2014, 9:08 a.m. UTC

The mmc-utils was modified to invoke eMMC5.0 Field Firmware Update (FFU) process in mmc driver
  New command was add: "do_emmc50_ffu".
  
  This patch depends on patch  mmc: Support-FFU-for-eMMC-v5.0 Committed by Avi Shchislowski <[e-mail]>
  
  FFU will be done in two steps. Two new IOCTL codes will be sent to the driver in order to operate FFU code:
    1.  FFU_DWONLOAD_OP (sent in ffu_download_image() function)
	2.  FFU_INSTALL_OP (sent in ffu_install() function)
  
  
Signed-off-by: Avi Shchislowski <[e-mail]>
Signed-off-by: Alex Lemberg <[e-mail]>

Avi Shchislowski | Staff Software Engineer, MCS Embedded  | SanDisk | +972.09.763-2666| www.sandisk.com


--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Grant Grundler Feb. 28, 2014, 10:39 p.m. UTC | #1

Avi,
Thanks for posting these - I look forward to seeing this functionality
available in mmc-utils (and kernel as needed).

Comments as usual inline.

I've added Gwendal/Kees to CC to comment on security issues of this
proposal. See notes below.


On Sun, Feb 9, 2014 at 1:08 AM, Avi Shchislowski
<[e-mail]> wrote:
>   The mmc-utils was modified to invoke eMMC5.0 Field Firmware Update (FFU) process in mmc driver
>   New command was add: "do_emmc50_ffu".
>
>   This patch depends on patch  mmc: Support-FFU-for-eMMC-v5.0 Committed by Avi Shchislowski <[e-mail]>
>
>   FFU will be done in two steps. Two new IOCTL codes will be sent to the driver in order to operate FFU code:
>     1.  FFU_DWONLOAD_OP (sent in ffu_download_image() function)

Any reason for the typo? DOWNLOAD maybe?
Shouldn't that be MMC_FFU_DOWNLOAD_OP to match the proposed kernel definition?

>         2.  FFU_INSTALL_OP (sent in ffu_install() function)

Ditto: MMC_FFU_INSTALL_OP

>
>
> Signed-off-by: Avi Shchislowski <[e-mail]>
> Signed-off-by: Alex Lemberg <[e-mail]>
>
> diff --git a/mmc.c b/mmc.c
> index 926e92f..a01852d 100644
> --- a/mmc.c
> +++ b/mmc.c
> @@ -36,9 +36,9 @@ struct Command {
>                                    if >= 0, number of arguments,
>                                    if < 0, _minimum_ number of arguments */
>         char    *verb;          /* verb */
> -       char    *help;          /* help lines; from the 2nd line onward they
> +       char    *help;          /* help lines; from the 2nd line onward they
>                                     are automatically indented */
> -        char    *adv_help;      /* advanced help message; from the 2nd line
> +        char    *adv_help;      /* advanced help message; from the 2nd line

Sorry, it's not obvious what changed here. Why is this included?

>                                     onward they are automatically indented */
>
>         /* the following fields are run-time filled by the program */ @@ -110,6 +110,11 @@ static struct Command commands[] = {
>                 "Send Sanitize command to the <device>.\nThis will delete the unmapped memory region of the device.",
>           NULL
>         },
> +       { do_emmc50_ffu, -2,
> +       "emmc50 ffu", "<image path> <device>\n"
> +         "run eMMC 5.0 Field firmware update.\n.",

Nit: This isn't "run". It's "download firmware to eMMC 5.0 compliant device".

> +         NULL
> +       },
>         { 0, 0, 0, 0 }
>  };
>
> @@ -362,7 +367,7 @@ static int parse_args(int argc, char **argv,
>                         matchcmd->verb, matchcmd->nargs);
>                         return -2;
>         }
> -
> +

I'm going to ignore white space mangle on this patch and assume you'll
ask if you need help using gmail to send patches using git send-email.

But the above isn't white space mangle caused by email - it's part of
this patch and I'm not seeing a difference in this <REDACTED> gmail
editor.

>          if (prepare_args( nargs_, args_, prgname, matchcmd )){
>                  fprintf(stderr, "ERROR: not enough memory\\n");
>                 return -20;
> diff --git a/mmc.h b/mmc.h
> index 9871d62..3be6db0 100644
> --- a/mmc.h
> +++ b/mmc.h
> @@ -80,6 +80,14 @@
>  #define BKOPS_ENABLE   (1<<0)
>
>  /*
> + * sector size
> +*/
> +#define CARD_BLOCK_SIZE        512

sector size is advertised by the device. It could be either 512 or 4K bytes. No?

"7.4.17 NUMBER_OF_FW_SECTORS_CORRECTLY_PROGRAMMED [305-302]

The value is in terms of 512 Bytes or in multiple of eight 512Bytes
sectors (4KBytes) depending on the value of the DATA_SECTOR_SIZE field."

I don't think this should be hard coded to 512. And a few places I see
hard coded with "<< 9" will likely need to take this into account.


> +
> +#define FFU_DWONLOAD_OP        302
> +#define FFU_INSTALL_OP 303

These should match kernel definitions (complete name and value).

> +
> +/*
>   * EXT_CSD field definitions
>   */
>  #define EXT_CSD_HPI_SUPP               (1<<0)
> diff --git a/mmc_cmds.c b/mmc_cmds.c
> index b8afa74..24c4a6b 100644
> --- a/mmc_cmds.c
> +++ b/mmc_cmds.c
> @@ -1163,3 +1163,112 @@ int do_sanitize(int nargs, char **argv)
>
>  }
>
> +static int ffu_download_image(int fw_fd, int mmc_fd) {
> +       int ret = 0;
> +       struct mmc_ioc_cmd mmc_ioc_cmd;
> +       char data_buff[MMC_IOC_MAX_BYTES];
> +       int file_size;

This should be off_t type. See "man 2 lseek".

> +       int size;

This should be size_t type.  See "man 2 read".

> +       int data_length;

This should ssize_t type. See "man 2 read".

> +
> +       memset(data_buff, 0, sizeof(data_buff));
> +       /* get file size */
> +       file_size = lseek(fw_fd, 0, SEEK_END);

I'm wondering why lseek would be preferred over fstat().

> +       if (file_size < 0) {
> +               ret =  -1;
> +               perror("seek file error \n");
> +               goto exit;
> +       }
> +
> +       lseek(fw_fd, 0, SEEK_SET);
> +       do {
> +               size = (file_size > MMC_IOC_MAX_BYTES) ?
> +                               MMC_IOC_MAX_BYTES : file_size;
> +               /* Read FW data from file */
> +               data_length = read(fw_fd, data_buff, size);
> +               if (data_length == -1) {

Should this test data_length < size ?

> +                       ret = -1;
> +                       goto exit;
> +               }

Gwendal and Kees (CC'd) would prefer to send the file name to the
kernel as part of the ioctl and use existing udev mechanisms to
request the firmware.

This has some advantages for security which make it a lot harder to
"plant" the hacked firmware on devices. I'll let Gwendal and Kees
present the details of those ideas.

> +               /* prepare and send ioctl */
> +               memset(&mmc_ioc_cmd, 0, sizeof(mmc_ioc_cmd));
> +               mmc_ioc_cmd.opcode =  FFU_DWONLOAD_OP;
> +               mmc_ioc_cmd.blksz = CARD_BLOCK_SIZE;
> +               mmc_ioc_cmd.blocks = data_length / mmc_ioc_cmd.blksz;
> +               mmc_ioc_cmd.arg =  0;
> +               mmc_ioc_cmd.flags = MMC_RSP_SPI_R1 | MMC_RSP_R1 | MMC_CMD_ADTC;
> +               mmc_ioc_cmd.write_flag = 1;
> +               mmc_ioc_cmd_set_data(mmc_ioc_cmd, data_buff);
> +               ret = ioctl(mmc_fd, MMC_IOC_CMD, &mmc_ioc_cmd);
> +               if (ret) {
> +                       perror("ioctl FW download");
> +                       goto exit;
> +               }
> +
> +               file_size = file_size - size;
> +               printf("firmware file loading, remaining:   %d\n", file_size);
> +       } while (file_size > 0);
> +
> +exit:
> +
> +       return ret;
> +}
> +
> +static int ffu_install(int mmc_fd)
> +{
> +       int ret;
> +       struct mmc_ioc_cmd mmc_ioc_cmd;
> +
> +       memset(&mmc_ioc_cmd, 0, sizeof(mmc_ioc_cmd));
> +       mmc_ioc_cmd.opcode = FFU_INSTALL_OP;
> +       mmc_ioc_cmd.blksz = CARD_BLOCK_SIZE;
> +       mmc_ioc_cmd.blocks = 0;
> +       mmc_ioc_cmd.arg =  0;
> +       mmc_ioc_cmd.flags = MMC_RSP_SPI_R1 | MMC_RSP_R1 | MMC_CMD_ADTC;
> +       mmc_ioc_cmd.write_flag = 0;
> +       ret = ioctl(mmc_fd, MMC_IOC_CMD, &mmc_ioc_cmd);
> +       if (ret)
> +               perror("ioctl install");
> +
> +       printf("ffu_install ret %d \n", ret);
> +       return ret;
> +}
> +
> +int do_emmc50_ffu(int nargs, char **argv) {
> +       int fd, fw_fd, ret;
> +       char *device;
> +
> +       CHECK(nargs != 3, "Usage: ffu <image path> </path/to/mmcblkX> \n",
> +                               exit(1));
> +
> +       device = argv[2];
> +       fd = open(device, O_RDWR);
> +       if (fd < 0) {
> +               perror("open");
> +               exit(1);
> +       }
> +
> +       /* open eMMC5.0 firmware image file */
> +       fw_fd = open(argv[1], O_RDONLY);
> +       if (fw_fd < 0) {
> +               perror("open eMMC5.0 firmware file");
> +               ret = -1;

Don't want to return the errno value?

> +               goto exit;
> +       }
> +
> +       ret = ffu_download_image(fw_fd, fd);
> +       if (ret)
> +               goto exit;
> +
> +       ret = ffu_install(fd);
> +       if (ret)
> +               goto exit;
> +
> +exit:
> +       close(fd);
> +       close(fw_fd);
> +
> +       return ret;
> +}
> diff --git a/mmc_cmds.h b/mmc_cmds.h
> index f06cc10..77a6cb8 100644
> --- a/mmc_cmds.h
> +++ b/mmc_cmds.h
> @@ -28,3 +28,5 @@ int do_sanitize(int nargs, char **argv);  int do_status_get(int nargs, char **argv);  int do_enh_area_set(int nargs, char **argv);  int do_write_reliability_set(int nargs, char **argv);
> +int do_emmc50_ffu(int nargs, char **argv);
> +
> --
> 1.7.5.4
>
>
> Avi Shchislowski | Staff Software Engineer, MCS Embedded  | SanDisk | +972.09.763-2666| www.sandisk.com
>

cheers,
grant
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Kees Cook Feb. 28, 2014, 11:20 p.m. UTC | #2

On Fri, Feb 28, 2014 at 2:39 PM, Grant Grundler <[e-mail]> wrote:
> Avi,
> Thanks for posting these - I look forward to seeing this functionality
> available in mmc-utils (and kernel as needed).
>
> Comments as usual inline.
>
> I've added Gwendal/Kees to CC to comment on security issues of this
> proposal. See notes below.
>
>
> On Sun, Feb 9, 2014 at 1:08 AM, Avi Shchislowski
> <[e-mail]> wrote:
>>   The mmc-utils was modified to invoke eMMC5.0 Field Firmware Update (FFU) process in mmc driver
>>   New command was add: "do_emmc50_ffu".
>>
>>   This patch depends on patch  mmc: Support-FFU-for-eMMC-v5.0 Committed by Avi Shchislowski <[e-mail]>
>>
>>   FFU will be done in two steps. Two new IOCTL codes will be sent to the driver in order to operate FFU code:
>>     1.  FFU_DWONLOAD_OP (sent in ffu_download_image() function)
>
> Any reason for the typo? DOWNLOAD maybe?
> Shouldn't that be MMC_FFU_DOWNLOAD_OP to match the proposed kernel definition?
>
>>         2.  FFU_INSTALL_OP (sent in ffu_install() function)
>
> Ditto: MMC_FFU_INSTALL_OP
>
>>
>>
>> Signed-off-by: Avi Shchislowski <[e-mail]>
>> Signed-off-by: Alex Lemberg <[e-mail]>
>>
>> diff --git a/mmc.c b/mmc.c
>> index 926e92f..a01852d 100644
>> --- a/mmc.c
>> +++ b/mmc.c
>> @@ -36,9 +36,9 @@ struct Command {
>>                                    if >= 0, number of arguments,
>>                                    if < 0, _minimum_ number of arguments */
>>         char    *verb;          /* verb */
>> -       char    *help;          /* help lines; from the 2nd line onward they
>> +       char    *help;          /* help lines; from the 2nd line onward they
>>                                     are automatically indented */
>> -        char    *adv_help;      /* advanced help message; from the 2nd line
>> +        char    *adv_help;      /* advanced help message; from the 2nd line
>
> Sorry, it's not obvious what changed here. Why is this included?
>
>>                                     onward they are automatically indented */
>>
>>         /* the following fields are run-time filled by the program */ @@ -110,6 +110,11 @@ static struct Command commands[] = {
>>                 "Send Sanitize command to the <device>.\nThis will delete the unmapped memory region of the device.",
>>           NULL
>>         },
>> +       { do_emmc50_ffu, -2,
>> +       "emmc50 ffu", "<image path> <device>\n"
>> +         "run eMMC 5.0 Field firmware update.\n.",
>
> Nit: This isn't "run". It's "download firmware to eMMC 5.0 compliant device".
>
>> +         NULL
>> +       },
>>         { 0, 0, 0, 0 }
>>  };
>>
>> @@ -362,7 +367,7 @@ static int parse_args(int argc, char **argv,
>>                         matchcmd->verb, matchcmd->nargs);
>>                         return -2;
>>         }
>> -
>> +
>
> I'm going to ignore white space mangle on this patch and assume you'll
> ask if you need help using gmail to send patches using git send-email.
>
> But the above isn't white space mangle caused by email - it's part of
> this patch and I'm not seeing a difference in this <REDACTED> gmail
> editor.
>
>>          if (prepare_args( nargs_, args_, prgname, matchcmd )){
>>                  fprintf(stderr, "ERROR: not enough memory\\n");
>>                 return -20;
>> diff --git a/mmc.h b/mmc.h
>> index 9871d62..3be6db0 100644
>> --- a/mmc.h
>> +++ b/mmc.h
>> @@ -80,6 +80,14 @@
>>  #define BKOPS_ENABLE   (1<<0)
>>
>>  /*
>> + * sector size
>> +*/
>> +#define CARD_BLOCK_SIZE        512
>
> sector size is advertised by the device. It could be either 512 or 4K bytes. No?
>
> "7.4.17 NUMBER_OF_FW_SECTORS_CORRECTLY_PROGRAMMED [305-302]
>
> The value is in terms of 512 Bytes or in multiple of eight 512Bytes
> sectors (4KBytes) depending on the value of the DATA_SECTOR_SIZE field."
>
> I don't think this should be hard coded to 512. And a few places I see
> hard coded with "<< 9" will likely need to take this into account.
>
>
>> +
>> +#define FFU_DWONLOAD_OP        302
>> +#define FFU_INSTALL_OP 303
>
> These should match kernel definitions (complete name and value).
>
>> +
>> +/*
>>   * EXT_CSD field definitions
>>   */
>>  #define EXT_CSD_HPI_SUPP               (1<<0)
>> diff --git a/mmc_cmds.c b/mmc_cmds.c
>> index b8afa74..24c4a6b 100644
>> --- a/mmc_cmds.c
>> +++ b/mmc_cmds.c
>> @@ -1163,3 +1163,112 @@ int do_sanitize(int nargs, char **argv)
>>
>>  }
>>
>> +static int ffu_download_image(int fw_fd, int mmc_fd) {
>> +       int ret = 0;
>> +       struct mmc_ioc_cmd mmc_ioc_cmd;
>> +       char data_buff[MMC_IOC_MAX_BYTES];
>> +       int file_size;
>
> This should be off_t type. See "man 2 lseek".
>
>> +       int size;
>
> This should be size_t type.  See "man 2 read".
>
>> +       int data_length;
>
> This should ssize_t type. See "man 2 read".
>
>> +
>> +       memset(data_buff, 0, sizeof(data_buff));
>> +       /* get file size */
>> +       file_size = lseek(fw_fd, 0, SEEK_END);
>
> I'm wondering why lseek would be preferred over fstat().
>
>> +       if (file_size < 0) {
>> +               ret =  -1;
>> +               perror("seek file error \n");
>> +               goto exit;
>> +       }
>> +
>> +       lseek(fw_fd, 0, SEEK_SET);
>> +       do {
>> +               size = (file_size > MMC_IOC_MAX_BYTES) ?
>> +                               MMC_IOC_MAX_BYTES : file_size;
>> +               /* Read FW data from file */
>> +               data_length = read(fw_fd, data_buff, size);
>> +               if (data_length == -1) {
>
> Should this test data_length < size ?
>
>> +                       ret = -1;
>> +                       goto exit;
>> +               }
>
> Gwendal and Kees (CC'd) would prefer to send the file name to the
> kernel as part of the ioctl and use existing udev mechanisms to
> request the firmware.

Yes, please see Documentation/firmware_class/README for information on
the kernel internals, but I would much prefer the kernel do all the
loading, not userspace. The kernel driver can request the firmware
contents:

         if(request_firmware(&fw_entry, $FIRMWARE, device) == 0)
                copy_fw_to_device(fw_entry->data, fw_entry->size);
         release(fw_entry);

and then send it to the device. Doing this from userspace means there
is no way to verify the firmware contents. Equally, the kernel should
actively block the MMC_FFU_DOWNLOAD_OP op, since it should be
considered a sensitive operation.

-Kees

>
> This has some advantages for security which make it a lot harder to
> "plant" the hacked firmware on devices. I'll let Gwendal and Kees
> present the details of those ideas.
>
>> +               /* prepare and send ioctl */
>> +               memset(&mmc_ioc_cmd, 0, sizeof(mmc_ioc_cmd));
>> +               mmc_ioc_cmd.opcode =  FFU_DWONLOAD_OP;
>> +               mmc_ioc_cmd.blksz = CARD_BLOCK_SIZE;
>> +               mmc_ioc_cmd.blocks = data_length / mmc_ioc_cmd.blksz;
>> +               mmc_ioc_cmd.arg =  0;
>> +               mmc_ioc_cmd.flags = MMC_RSP_SPI_R1 | MMC_RSP_R1 | MMC_CMD_ADTC;
>> +               mmc_ioc_cmd.write_flag = 1;
>> +               mmc_ioc_cmd_set_data(mmc_ioc_cmd, data_buff);
>> +               ret = ioctl(mmc_fd, MMC_IOC_CMD, &mmc_ioc_cmd);
>> +               if (ret) {
>> +                       perror("ioctl FW download");
>> +                       goto exit;
>> +               }
>> +
>> +               file_size = file_size - size;
>> +               printf("firmware file loading, remaining:   %d\n", file_size);
>> +       } while (file_size > 0);
>> +
>> +exit:
>> +
>> +       return ret;
>> +}
>> +
>> +static int ffu_install(int mmc_fd)
>> +{
>> +       int ret;
>> +       struct mmc_ioc_cmd mmc_ioc_cmd;
>> +
>> +       memset(&mmc_ioc_cmd, 0, sizeof(mmc_ioc_cmd));
>> +       mmc_ioc_cmd.opcode = FFU_INSTALL_OP;
>> +       mmc_ioc_cmd.blksz = CARD_BLOCK_SIZE;
>> +       mmc_ioc_cmd.blocks = 0;
>> +       mmc_ioc_cmd.arg =  0;
>> +       mmc_ioc_cmd.flags = MMC_RSP_SPI_R1 | MMC_RSP_R1 | MMC_CMD_ADTC;
>> +       mmc_ioc_cmd.write_flag = 0;
>> +       ret = ioctl(mmc_fd, MMC_IOC_CMD, &mmc_ioc_cmd);
>> +       if (ret)
>> +               perror("ioctl install");
>> +
>> +       printf("ffu_install ret %d \n", ret);
>> +       return ret;
>> +}
>> +
>> +int do_emmc50_ffu(int nargs, char **argv) {
>> +       int fd, fw_fd, ret;
>> +       char *device;
>> +
>> +       CHECK(nargs != 3, "Usage: ffu <image path> </path/to/mmcblkX> \n",
>> +                               exit(1));
>> +
>> +       device = argv[2];
>> +       fd = open(device, O_RDWR);
>> +       if (fd < 0) {
>> +               perror("open");
>> +               exit(1);
>> +       }
>> +
>> +       /* open eMMC5.0 firmware image file */
>> +       fw_fd = open(argv[1], O_RDONLY);
>> +       if (fw_fd < 0) {
>> +               perror("open eMMC5.0 firmware file");
>> +               ret = -1;
>
> Don't want to return the errno value?
>
>> +               goto exit;
>> +       }
>> +
>> +       ret = ffu_download_image(fw_fd, fd);
>> +       if (ret)
>> +               goto exit;
>> +
>> +       ret = ffu_install(fd);
>> +       if (ret)
>> +               goto exit;
>> +
>> +exit:
>> +       close(fd);
>> +       close(fw_fd);
>> +
>> +       return ret;
>> +}
>> diff --git a/mmc_cmds.h b/mmc_cmds.h
>> index f06cc10..77a6cb8 100644
>> --- a/mmc_cmds.h
>> +++ b/mmc_cmds.h
>> @@ -28,3 +28,5 @@ int do_sanitize(int nargs, char **argv);  int do_status_get(int nargs, char **argv);  int do_enh_area_set(int nargs, char **argv);  int do_write_reliability_set(int nargs, char **argv);
>> +int do_emmc50_ffu(int nargs, char **argv);
>> +
>> --
>> 1.7.5.4
>>
>>
>> Avi Shchislowski | Staff Software Engineer, MCS Embedded  | SanDisk | +972.09.763-2666| www.sandisk.com
>>
>
> cheers,
> grant

Alex Lemberg March 5, 2014, 3:24 p.m. UTC | #3

SGkgR3JhbnQsDQoNClRoYW5rcyBmb3IgeW91ciByZXZpZXcgYW5kIGNvbW1lbnRzLg0KUGxlYXNl
IHNlZSBvdXIgcmVzcG9uc2UgaW5saW5lLg0KDQoNCg0KPiAtLS0tLU9yaWdpbmFsIE1lc3NhZ2Ut
LS0tLQ0KPiBGcm9tOiBncnVuZGxlckBnb29nbGUuY29tIFttYWlsdG86Z3J1bmRsZXJAZ29vZ2xl
LmNvbV0gT24gQmVoYWxmIE9mDQo+IEdyYW50IEdydW5kbGVyDQo+IFNlbnQ6IFNhdHVyZGF5LCBN
YXJjaCAwMSwgMjAxNCAxMjo0MCBBTQ0KPiBUbzogQXZpIFNoY2hpc2xvd3NraQ0KPiBDYzogY2pi
QGxhcHRvcC5vcmc7IGxpbnV4LW1tY0B2Z2VyLmtlcm5lbC5vcmc7IEFsZXggTGVtYmVyZzsgR3Jh
bnQNCj4gR3J1bmRsZXI7IEd3ZW5kYWwgR3JpZ25vdTsgUHV0aGlrb3JuIFZvcmF2b290aXZhdDsg
S2VlcyBDb29rDQo+IFN1YmplY3Q6IFJlOiBbUkZDIFBBVENIIDEvMV0gbW1jLXV0aWxzOiBTdXBw
b3J0LXNlbmRpbmctZU1NQy01LjAtRkZVDQo+IA0KPiBBdmksDQo+IFRoYW5rcyBmb3IgcG9zdGlu
ZyB0aGVzZSAtIEkgbG9vayBmb3J3YXJkIHRvIHNlZWluZyB0aGlzIGZ1bmN0aW9uYWxpdHkgYXZh
aWxhYmxlDQo+IGluIG1tYy11dGlscyAoYW5kIGtlcm5lbCBhcyBuZWVkZWQpLg0KPiANCj4gQ29t
bWVudHMgYXMgdXN1YWwgaW5saW5lLg0KPiANCj4gSSd2ZSBhZGRlZCBHd2VuZGFsL0tlZXMgdG8g
Q0MgdG8gY29tbWVudCBvbiBzZWN1cml0eSBpc3N1ZXMgb2YgdGhpcw0KPiBwcm9wb3NhbC4gU2Vl
IG5vdGVzIGJlbG93Lg0KPiANCj4gDQo+IE9uIFN1biwgRmViIDksIDIwMTQgYXQgMTowOCBBTSwg
QXZpIFNoY2hpc2xvd3NraQ0KPiA8QXZpLlNoY2hpc2xvd3NraUBzYW5kaXNrLmNvbT4gd3JvdGU6
DQo+ID4gICBUaGUgbW1jLXV0aWxzIHdhcyBtb2RpZmllZCB0byBpbnZva2UgZU1NQzUuMCBGaWVs
ZCBGaXJtd2FyZSBVcGRhdGUNCj4gKEZGVSkgcHJvY2VzcyBpbiBtbWMgZHJpdmVyDQo+ID4gICBO
ZXcgY29tbWFuZCB3YXMgYWRkOiAiZG9fZW1tYzUwX2ZmdSIuDQo+ID4NCj4gPiAgIFRoaXMgcGF0
Y2ggZGVwZW5kcyBvbiBwYXRjaCAgbW1jOiBTdXBwb3J0LUZGVS1mb3ItZU1NQy12NS4wDQo+ID4g
Q29tbWl0dGVkIGJ5IEF2aSBTaGNoaXNsb3dza2kgPGF2aS5zaGNoaXNsb3dza2lAc2FuZGlzay5j
b20+DQo+ID4NCj4gPiAgIEZGVSB3aWxsIGJlIGRvbmUgaW4gdHdvIHN0ZXBzLiBUd28gbmV3IElP
Q1RMIGNvZGVzIHdpbGwgYmUgc2VudCB0byB0aGUNCj4gZHJpdmVyIGluIG9yZGVyIHRvIG9wZXJh
dGUgRkZVIGNvZGU6DQo+ID4gICAgIDEuICBGRlVfRFdPTkxPQURfT1AgKHNlbnQgaW4gZmZ1X2Rv
d25sb2FkX2ltYWdlKCkgZnVuY3Rpb24pDQo+IA0KVHlwbw0KDQo+IEFueSByZWFzb24gZm9yIHRo
ZSB0eXBvPyBET1dOTE9BRCBtYXliZT8NCj4gU2hvdWxkbid0IHRoYXQgYmUgTU1DX0ZGVV9ET1dO
TE9BRF9PUCB0byBtYXRjaCB0aGUgcHJvcG9zZWQga2VybmVsDQo+IGRlZmluaXRpb24/DQoNCkFn
cmVlLiBDaGFuZ2luZyB0byBNTUNfRkZVX0RPV05MT0FEX09QLg0KPiANCj4gPiAgICAgICAgIDIu
ICBGRlVfSU5TVEFMTF9PUCAoc2VudCBpbiBmZnVfaW5zdGFsbCgpIGZ1bmN0aW9uKQ0KPiANCj4g
RGl0dG86IE1NQ19GRlVfSU5TVEFMTF9PUA0KPiANCkFncmVlLiBDaGFuZ2luZyB0byBNTUNfRkZV
X0lOU1RBTExfT1AuDQo+ID4NCj4gPg0KPiA+IFNpZ25lZC1vZmYtYnk6IEF2aSBTaGNoaXNsb3dz
a2kgPGF2aS5zaGNoaXNsb3dza2lAc2FuZGlzay5jb20+DQo+ID4gU2lnbmVkLW9mZi1ieTogQWxl
eCBMZW1iZXJnIDxhbGV4LmxlbWJlcmdAc2FuZGlzay5jb20+DQo+ID4NCj4gPiBkaWZmIC0tZ2l0
IGEvbW1jLmMgYi9tbWMuYw0KPiA+IGluZGV4IDkyNmU5MmYuLmEwMTg1MmQgMTAwNjQ0DQo+ID4g
LS0tIGEvbW1jLmMNCj4gPiArKysgYi9tbWMuYw0KPiA+IEBAIC0zNiw5ICszNiw5IEBAIHN0cnVj
dCBDb21tYW5kIHsNCj4gPiAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgIGlmID49
IDAsIG51bWJlciBvZiBhcmd1bWVudHMsDQo+ID4gICAgICAgICAgICAgICAgICAgICAgICAgICAg
ICAgICAgICBpZiA8IDAsIF9taW5pbXVtXyBudW1iZXIgb2YgYXJndW1lbnRzICovDQo+ID4gICAg
ICAgICBjaGFyICAgICp2ZXJiOyAgICAgICAgICAvKiB2ZXJiICovDQo+ID4gLSAgICAgICBjaGFy
ICAgICpoZWxwOyAgICAgICAgICAvKiBoZWxwIGxpbmVzOyBmcm9tIHRoZSAybmQgbGluZSBvbndh
cmQgdGhleQ0KPiA+ICsgICAgICAgY2hhciAgICAqaGVscDsgICAgICAgICAgLyogaGVscCBsaW5l
czsgZnJvbSB0aGUgMm5kIGxpbmUgb253YXJkIHRoZXkNCj4gPiAgICAgICAgICAgICAgICAgICAg
ICAgICAgICAgICAgICAgICBhcmUgYXV0b21hdGljYWxseSBpbmRlbnRlZCAqLw0KPiA+IC0gICAg
ICAgIGNoYXIgICAgKmFkdl9oZWxwOyAgICAgIC8qIGFkdmFuY2VkIGhlbHAgbWVzc2FnZTsgZnJv
bSB0aGUgMm5kIGxpbmUNCj4gPiArICAgICAgICBjaGFyICAgICphZHZfaGVscDsgICAgICAvKiBh
ZHZhbmNlZCBoZWxwIG1lc3NhZ2U7IGZyb20gdGhlIDJuZCBsaW5lDQo+IA0KPiBTb3JyeSwgaXQn
cyBub3Qgb2J2aW91cyB3aGF0IGNoYW5nZWQgaGVyZS4gV2h5IGlzIHRoaXMgaW5jbHVkZWQ/DQo+
IA0KPiA+ICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgIG9ud2FyZCB0aGV5IGFy
ZSBhdXRvbWF0aWNhbGx5DQo+ID4gaW5kZW50ZWQgKi8NCg0KV2Ugd2lsbCByZW1vdmUgdGhpcy4N
Cj4gPg0KPiA+ICAgICAgICAgLyogdGhlIGZvbGxvd2luZyBmaWVsZHMgYXJlIHJ1bi10aW1lIGZp
bGxlZCBieSB0aGUgcHJvZ3JhbSAqLyBAQCAtMTEwLDYNCj4gKzExMCwxMSBAQCBzdGF0aWMgc3Ry
dWN0IENvbW1hbmQgY29tbWFuZHNbXSA9IHsNCj4gPiAgICAgICAgICAgICAgICAgIlNlbmQgU2Fu
aXRpemUgY29tbWFuZCB0byB0aGUgPGRldmljZT4uXG5UaGlzIHdpbGwgZGVsZXRlIHRoZQ0KPiB1
bm1hcHBlZCBtZW1vcnkgcmVnaW9uIG9mIHRoZSBkZXZpY2UuIiwNCj4gPiAgICAgICAgICAgTlVM
TA0KPiA+ICAgICAgICAgfSwNCj4gPiArICAgICAgIHsgZG9fZW1tYzUwX2ZmdSwgLTIsDQo+ID4g
KyAgICAgICAiZW1tYzUwIGZmdSIsICI8aW1hZ2UgcGF0aD4gPGRldmljZT5cbiINCj4gPiArICAg
ICAgICAgInJ1biBlTU1DIDUuMCBGaWVsZCBmaXJtd2FyZSB1cGRhdGUuXG4uIiwNCj4gDQo+IE5p
dDogVGhpcyBpc24ndCAicnVuIi4gSXQncyAiZG93bmxvYWQgZmlybXdhcmUgdG8gZU1NQyA1LjAg
Y29tcGxpYW50IGRldmljZSIuDQpPSw0KPiANCj4gPiArICAgICAgICAgTlVMTA0KPiA+ICsgICAg
ICAgfSwNCj4gPiAgICAgICAgIHsgMCwgMCwgMCwgMCB9DQo+ID4gIH07DQo+ID4NCj4gPiBAQCAt
MzYyLDcgKzM2Nyw3IEBAIHN0YXRpYyBpbnQgcGFyc2VfYXJncyhpbnQgYXJnYywgY2hhciAqKmFy
Z3YsDQo+ID4gICAgICAgICAgICAgICAgICAgICAgICAgbWF0Y2hjbWQtPnZlcmIsIG1hdGNoY21k
LT5uYXJncyk7DQo+ID4gICAgICAgICAgICAgICAgICAgICAgICAgcmV0dXJuIC0yOw0KPiA+ICAg
ICAgICAgfQ0KPiA+IC0NCj4gPiArDQo+IA0KPiBJJ20gZ29pbmcgdG8gaWdub3JlIHdoaXRlIHNw
YWNlIG1hbmdsZSBvbiB0aGlzIHBhdGNoIGFuZCBhc3N1bWUgeW91J2xsIGFzayBpZg0KPiB5b3Ug
bmVlZCBoZWxwIHVzaW5nIGdtYWlsIHRvIHNlbmQgcGF0Y2hlcyB1c2luZyBnaXQgc2VuZC1lbWFp
bC4NCj4gDQo+IEJ1dCB0aGUgYWJvdmUgaXNuJ3Qgd2hpdGUgc3BhY2UgbWFuZ2xlIGNhdXNlZCBi
eSBlbWFpbCAtIGl0J3MgcGFydCBvZiB0aGlzDQo+IHBhdGNoIGFuZCBJJ20gbm90IHNlZWluZyBh
IGRpZmZlcmVuY2UgaW4gdGhpcyA8UkVEQUNURUQ+IGdtYWlsIGVkaXRvci4NClRoYW5rcywgd2Ug
YXJlIGNoZWNraW5nIHRoaXMuLi4NCj4gDQo+ID4gICAgICAgICAgaWYgKHByZXBhcmVfYXJncygg
bmFyZ3NfLCBhcmdzXywgcHJnbmFtZSwgbWF0Y2hjbWQgKSl7DQo+ID4gICAgICAgICAgICAgICAg
ICBmcHJpbnRmKHN0ZGVyciwgIkVSUk9SOiBub3QgZW5vdWdoIG1lbW9yeVxcbiIpOw0KPiA+ICAg
ICAgICAgICAgICAgICByZXR1cm4gLTIwOw0KPiA+IGRpZmYgLS1naXQgYS9tbWMuaCBiL21tYy5o
DQo+ID4gaW5kZXggOTg3MWQ2Mi4uM2JlNmRiMCAxMDA2NDQNCj4gPiAtLS0gYS9tbWMuaA0KPiA+
ICsrKyBiL21tYy5oDQo+ID4gQEAgLTgwLDYgKzgwLDE0IEBADQo+ID4gICNkZWZpbmUgQktPUFNf
RU5BQkxFICAgKDE8PDApDQo+ID4NCj4gPiAgLyoNCj4gPiArICogc2VjdG9yIHNpemUNCj4gPiAr
Ki8NCj4gPiArI2RlZmluZSBDQVJEX0JMT0NLX1NJWkUgICAgICAgIDUxMg0KPiANCj4gc2VjdG9y
IHNpemUgaXMgYWR2ZXJ0aXNlZCBieSB0aGUgZGV2aWNlLiBJdCBjb3VsZCBiZSBlaXRoZXIgNTEy
IG9yIDRLIGJ5dGVzLiBObz8NCj4gDQo+ICI3LjQuMTcgTlVNQkVSX09GX0ZXX1NFQ1RPUlNfQ09S
UkVDVExZX1BST0dSQU1NRUQgWzMwNS0zMDJdDQo+IA0KPiBUaGUgdmFsdWUgaXMgaW4gdGVybXMg
b2YgNTEyIEJ5dGVzIG9yIGluIG11bHRpcGxlIG9mIGVpZ2h0IDUxMkJ5dGVzIHNlY3RvcnMNCj4g
KDRLQnl0ZXMpIGRlcGVuZGluZyBvbiB0aGUgdmFsdWUgb2YgdGhlIERBVEFfU0VDVE9SX1NJWkUg
ZmllbGQuIg0KPiANCj4gSSBkb24ndCB0aGluayB0aGlzIHNob3VsZCBiZSBoYXJkIGNvZGVkIHRv
IDUxMi4gQW5kIGEgZmV3IHBsYWNlcyBJIHNlZSBoYXJkDQo+IGNvZGVkIHdpdGggIjw8IDkiIHdp
bGwgbGlrZWx5IG5lZWQgdG8gdGFrZSB0aGlzIGludG8gYWNjb3VudC4NCg0KQWdyZWUuIFdlIHdp
bGwgc2V0IHRoaXMgdmFsdWUgZnJvbSB0aGUgRVhUX0NTRF9EQVRBX1NFQ1RPUl9TSVpFIChQbGVh
c2Ugbm90ZSAtIHRoaXMgcmVxdWlyZXMgYWRkaXRpb25hbCBvcGVyYXRpb24gb2YgcmVhZGluZyBF
WFRfQ1NEKS4NCg0KPiANCj4gDQo+ID4gKw0KPiA+ICsjZGVmaW5lIEZGVV9EV09OTE9BRF9PUCAg
ICAgICAgMzAyDQo+ID4gKyNkZWZpbmUgRkZVX0lOU1RBTExfT1AgMzAzDQo+IA0KPiBUaGVzZSBz
aG91bGQgbWF0Y2gga2VybmVsIGRlZmluaXRpb25zIChjb21wbGV0ZSBuYW1lIGFuZCB2YWx1ZSku
DQpBZ3JlZQ0KDQo+IA0KPiA+ICsNCj4gPiArLyoNCj4gPiAgICogRVhUX0NTRCBmaWVsZCBkZWZp
bml0aW9ucw0KPiA+ICAgKi8NCj4gPiAgI2RlZmluZSBFWFRfQ1NEX0hQSV9TVVBQICAgICAgICAg
ICAgICAgKDE8PDApDQo+ID4gZGlmZiAtLWdpdCBhL21tY19jbWRzLmMgYi9tbWNfY21kcy5jDQo+
ID4gaW5kZXggYjhhZmE3NC4uMjRjNGE2YiAxMDA2NDQNCj4gPiAtLS0gYS9tbWNfY21kcy5jDQo+
ID4gKysrIGIvbW1jX2NtZHMuYw0KPiA+IEBAIC0xMTYzLDMgKzExNjMsMTEyIEBAIGludCBkb19z
YW5pdGl6ZShpbnQgbmFyZ3MsIGNoYXIgKiphcmd2KQ0KPiA+DQo+ID4gIH0NCj4gPg0KPiA+ICtz
dGF0aWMgaW50IGZmdV9kb3dubG9hZF9pbWFnZShpbnQgZndfZmQsIGludCBtbWNfZmQpIHsNCj4g
PiArICAgICAgIGludCByZXQgPSAwOw0KPiA+ICsgICAgICAgc3RydWN0IG1tY19pb2NfY21kIG1t
Y19pb2NfY21kOw0KPiA+ICsgICAgICAgY2hhciBkYXRhX2J1ZmZbTU1DX0lPQ19NQVhfQllURVNd
Ow0KPiA+ICsgICAgICAgaW50IGZpbGVfc2l6ZTsNCj4gDQo+IFRoaXMgc2hvdWxkIGJlIG9mZl90
IHR5cGUuIFNlZSAibWFuIDIgbHNlZWsiLg0KUmlnaHQNCg0KPiANCj4gPiArICAgICAgIGludCBz
aXplOw0KPiANCj4gVGhpcyBzaG91bGQgYmUgc2l6ZV90IHR5cGUuICBTZWUgIm1hbiAyIHJlYWQi
Lg0KUmlnaHQNCj4gDQo+ID4gKyAgICAgICBpbnQgZGF0YV9sZW5ndGg7DQo+IA0KPiBUaGlzIHNo
b3VsZCBzc2l6ZV90IHR5cGUuIFNlZSAibWFuIDIgcmVhZCIuDQpSaWdodA0KDQo+IA0KPiA+ICsN
Cj4gPiArICAgICAgIG1lbXNldChkYXRhX2J1ZmYsIDAsIHNpemVvZihkYXRhX2J1ZmYpKTsNCj4g
PiArICAgICAgIC8qIGdldCBmaWxlIHNpemUgKi8NCj4gPiArICAgICAgIGZpbGVfc2l6ZSA9IGxz
ZWVrKGZ3X2ZkLCAwLCBTRUVLX0VORCk7DQo+IA0KPiBJJ20gd29uZGVyaW5nIHdoeSBsc2VlayB3
b3VsZCBiZSBwcmVmZXJyZWQgb3ZlciBmc3RhdCgpLg0KTm8gc3BlY2lhbCByZWFzb24uIENoYW5n
aW5nIHRoaXMgdG8gdXNlIGZzdGF0KCkuDQo+IA0KPiA+ICsgICAgICAgaWYgKGZpbGVfc2l6ZSA8
IDApIHsNCj4gPiArICAgICAgICAgICAgICAgcmV0ID0gIC0xOw0KPiA+ICsgICAgICAgICAgICAg
ICBwZXJyb3IoInNlZWsgZmlsZSBlcnJvciBcbiIpOw0KPiA+ICsgICAgICAgICAgICAgICBnb3Rv
IGV4aXQ7DQo+ID4gKyAgICAgICB9DQo+ID4gKw0KPiA+ICsgICAgICAgbHNlZWsoZndfZmQsIDAs
IFNFRUtfU0VUKTsNCj4gPiArICAgICAgIGRvIHsNCj4gPiArICAgICAgICAgICAgICAgc2l6ZSA9
IChmaWxlX3NpemUgPiBNTUNfSU9DX01BWF9CWVRFUykgPw0KPiA+ICsgICAgICAgICAgICAgICAg
ICAgICAgICAgICAgICAgTU1DX0lPQ19NQVhfQllURVMgOiBmaWxlX3NpemU7DQo+ID4gKyAgICAg
ICAgICAgICAgIC8qIFJlYWQgRlcgZGF0YSBmcm9tIGZpbGUgKi8NCj4gPiArICAgICAgICAgICAg
ICAgZGF0YV9sZW5ndGggPSByZWFkKGZ3X2ZkLCBkYXRhX2J1ZmYsIHNpemUpOw0KPiA+ICsgICAg
ICAgICAgICAgICBpZiAoZGF0YV9sZW5ndGggPT0gLTEpIHsNCj4gDQo+IFNob3VsZCB0aGlzIHRl
c3QgZGF0YV9sZW5ndGggPCBzaXplID8NCg0KDQpJbiBjYXNlIHdlIGNhbid0IHJlbHkgb24gc3Rh
dHVzIHJldHVybmVkIGJ5IHJlYWQoKSwgdGhpcyBjb25kaXRpb24gKGRhdGFfbGVuZ3RoIDwgc2l6
ZSkgaXMgYmV0dGVyLg0KDQo+IA0KPiA+ICsgICAgICAgICAgICAgICAgICAgICAgIHJldCA9IC0x
Ow0KPiA+ICsgICAgICAgICAgICAgICAgICAgICAgIGdvdG8gZXhpdDsNCj4gPiArICAgICAgICAg
ICAgICAgfQ0KPiANCj4gR3dlbmRhbCBhbmQgS2VlcyAoQ0MnZCkgd291bGQgcHJlZmVyIHRvIHNl
bmQgdGhlIGZpbGUgbmFtZSB0byB0aGUga2VybmVsIGFzDQo+IHBhcnQgb2YgdGhlIGlvY3RsIGFu
ZCB1c2UgZXhpc3RpbmcgdWRldiBtZWNoYW5pc21zIHRvIHJlcXVlc3QgdGhlIGZpcm13YXJlLg0K
PiANCj4gVGhpcyBoYXMgc29tZSBhZHZhbnRhZ2VzIGZvciBzZWN1cml0eSB3aGljaCBtYWtlIGl0
IGEgbG90IGhhcmRlciB0byAicGxhbnQiDQo+IHRoZSBoYWNrZWQgZmlybXdhcmUgb24gZGV2aWNl
cy4gSSdsbCBsZXQgR3dlbmRhbCBhbmQgS2VlcyBwcmVzZW50IHRoZSBkZXRhaWxzDQo+IG9mIHRo
b3NlIGlkZWFzLg0KDQpPSywgV2Ugd2lsbCByZWZlciB0byBHd2VuZGFsIGFuZCBLZWVzIGNvbW1l
bnRzLg0KDQo+IA0KPiA+ICsgICAgICAgICAgICAgICAvKiBwcmVwYXJlIGFuZCBzZW5kIGlvY3Rs
ICovDQo+ID4gKyAgICAgICAgICAgICAgIG1lbXNldCgmbW1jX2lvY19jbWQsIDAsIHNpemVvZiht
bWNfaW9jX2NtZCkpOw0KPiA+ICsgICAgICAgICAgICAgICBtbWNfaW9jX2NtZC5vcGNvZGUgPSAg
RkZVX0RXT05MT0FEX09QOw0KPiA+ICsgICAgICAgICAgICAgICBtbWNfaW9jX2NtZC5ibGtzeiA9
IENBUkRfQkxPQ0tfU0laRTsNCj4gPiArICAgICAgICAgICAgICAgbW1jX2lvY19jbWQuYmxvY2tz
ID0gZGF0YV9sZW5ndGggLyBtbWNfaW9jX2NtZC5ibGtzejsNCj4gPiArICAgICAgICAgICAgICAg
bW1jX2lvY19jbWQuYXJnID0gIDA7DQo+ID4gKyAgICAgICAgICAgICAgIG1tY19pb2NfY21kLmZs
YWdzID0gTU1DX1JTUF9TUElfUjEgfCBNTUNfUlNQX1IxIHwNCj4gTU1DX0NNRF9BRFRDOw0KPiA+
ICsgICAgICAgICAgICAgICBtbWNfaW9jX2NtZC53cml0ZV9mbGFnID0gMTsNCj4gPiArICAgICAg
ICAgICAgICAgbW1jX2lvY19jbWRfc2V0X2RhdGEobW1jX2lvY19jbWQsIGRhdGFfYnVmZik7DQo+
ID4gKyAgICAgICAgICAgICAgIHJldCA9IGlvY3RsKG1tY19mZCwgTU1DX0lPQ19DTUQsICZtbWNf
aW9jX2NtZCk7DQo+ID4gKyAgICAgICAgICAgICAgIGlmIChyZXQpIHsNCj4gPiArICAgICAgICAg
ICAgICAgICAgICAgICBwZXJyb3IoImlvY3RsIEZXIGRvd25sb2FkIik7DQo+ID4gKyAgICAgICAg
ICAgICAgICAgICAgICAgZ290byBleGl0Ow0KPiA+ICsgICAgICAgICAgICAgICB9DQo+ID4gKw0K
PiA+ICsgICAgICAgICAgICAgICBmaWxlX3NpemUgPSBmaWxlX3NpemUgLSBzaXplOw0KPiA+ICsg
ICAgICAgICAgICAgICBwcmludGYoImZpcm13YXJlIGZpbGUgbG9hZGluZywgcmVtYWluaW5nOiAg
ICVkXG4iLCBmaWxlX3NpemUpOw0KPiA+ICsgICAgICAgfSB3aGlsZSAoZmlsZV9zaXplID4gMCk7
DQo+ID4gKw0KPiA+ICtleGl0Og0KPiA+ICsNCj4gPiArICAgICAgIHJldHVybiByZXQ7DQo+ID4g
K30NCj4gPiArDQo+ID4gK3N0YXRpYyBpbnQgZmZ1X2luc3RhbGwoaW50IG1tY19mZCkNCj4gPiAr
ew0KPiA+ICsgICAgICAgaW50IHJldDsNCj4gPiArICAgICAgIHN0cnVjdCBtbWNfaW9jX2NtZCBt
bWNfaW9jX2NtZDsNCj4gPiArDQo+ID4gKyAgICAgICBtZW1zZXQoJm1tY19pb2NfY21kLCAwLCBz
aXplb2YobW1jX2lvY19jbWQpKTsNCj4gPiArICAgICAgIG1tY19pb2NfY21kLm9wY29kZSA9IEZG
VV9JTlNUQUxMX09QOw0KPiA+ICsgICAgICAgbW1jX2lvY19jbWQuYmxrc3ogPSBDQVJEX0JMT0NL
X1NJWkU7DQo+ID4gKyAgICAgICBtbWNfaW9jX2NtZC5ibG9ja3MgPSAwOw0KPiA+ICsgICAgICAg
bW1jX2lvY19jbWQuYXJnID0gIDA7DQo+ID4gKyAgICAgICBtbWNfaW9jX2NtZC5mbGFncyA9IE1N
Q19SU1BfU1BJX1IxIHwgTU1DX1JTUF9SMSB8DQo+IE1NQ19DTURfQURUQzsNCj4gPiArICAgICAg
IG1tY19pb2NfY21kLndyaXRlX2ZsYWcgPSAwOw0KPiA+ICsgICAgICAgcmV0ID0gaW9jdGwobW1j
X2ZkLCBNTUNfSU9DX0NNRCwgJm1tY19pb2NfY21kKTsNCj4gPiArICAgICAgIGlmIChyZXQpDQo+
ID4gKyAgICAgICAgICAgICAgIHBlcnJvcigiaW9jdGwgaW5zdGFsbCIpOw0KPiA+ICsNCj4gPiAr
ICAgICAgIHByaW50ZigiZmZ1X2luc3RhbGwgcmV0ICVkIFxuIiwgcmV0KTsNCj4gPiArICAgICAg
IHJldHVybiByZXQ7DQo+ID4gK30NCj4gPiArDQo+ID4gK2ludCBkb19lbW1jNTBfZmZ1KGludCBu
YXJncywgY2hhciAqKmFyZ3YpIHsNCj4gPiArICAgICAgIGludCBmZCwgZndfZmQsIHJldDsNCj4g
PiArICAgICAgIGNoYXIgKmRldmljZTsNCj4gPiArDQo+ID4gKyAgICAgICBDSEVDSyhuYXJncyAh
PSAzLCAiVXNhZ2U6IGZmdSA8aW1hZ2UgcGF0aD4gPC9wYXRoL3RvL21tY2Jsa1g+IFxuIiwNCj4g
PiArICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgIGV4aXQoMSkpOw0KPiA+ICsNCj4gPiAr
ICAgICAgIGRldmljZSA9IGFyZ3ZbMl07DQo+ID4gKyAgICAgICBmZCA9IG9wZW4oZGV2aWNlLCBP
X1JEV1IpOw0KPiA+ICsgICAgICAgaWYgKGZkIDwgMCkgew0KPiA+ICsgICAgICAgICAgICAgICBw
ZXJyb3IoIm9wZW4iKTsNCj4gPiArICAgICAgICAgICAgICAgZXhpdCgxKTsNCj4gPiArICAgICAg
IH0NCj4gPiArDQo+ID4gKyAgICAgICAvKiBvcGVuIGVNTUM1LjAgZmlybXdhcmUgaW1hZ2UgZmls
ZSAqLw0KPiA+ICsgICAgICAgZndfZmQgPSBvcGVuKGFyZ3ZbMV0sIE9fUkRPTkxZKTsNCj4gPiAr
ICAgICAgIGlmIChmd19mZCA8IDApIHsNCj4gPiArICAgICAgICAgICAgICAgcGVycm9yKCJvcGVu
IGVNTUM1LjAgZmlybXdhcmUgZmlsZSIpOw0KPiA+ICsgICAgICAgICAgICAgICByZXQgPSAtMTsN
Cj4gDQo+IERvbid0IHdhbnQgdG8gcmV0dXJuIHRoZSBlcnJubyB2YWx1ZT8NCk9LLiBXaWxsIGRv
Lg0KDQo+IA0KPiA+ICsgICAgICAgICAgICAgICBnb3RvIGV4aXQ7DQo+ID4gKyAgICAgICB9DQo+
ID4gKw0KPiA+ICsgICAgICAgcmV0ID0gZmZ1X2Rvd25sb2FkX2ltYWdlKGZ3X2ZkLCBmZCk7DQo+
ID4gKyAgICAgICBpZiAocmV0KQ0KPiA+ICsgICAgICAgICAgICAgICBnb3RvIGV4aXQ7DQo+ID4g
Kw0KPiA+ICsgICAgICAgcmV0ID0gZmZ1X2luc3RhbGwoZmQpOw0KPiA+ICsgICAgICAgaWYgKHJl
dCkNCj4gPiArICAgICAgICAgICAgICAgZ290byBleGl0Ow0KPiA+ICsNCj4gPiArZXhpdDoNCj4g
PiArICAgICAgIGNsb3NlKGZkKTsNCj4gPiArICAgICAgIGNsb3NlKGZ3X2ZkKTsNCj4gPiArDQo+
ID4gKyAgICAgICByZXR1cm4gcmV0Ow0KPiA+ICt9DQo+ID4gZGlmZiAtLWdpdCBhL21tY19jbWRz
LmggYi9tbWNfY21kcy5oDQo+ID4gaW5kZXggZjA2Y2MxMC4uNzdhNmNiOCAxMDA2NDQNCj4gPiAt
LS0gYS9tbWNfY21kcy5oDQo+ID4gKysrIGIvbW1jX2NtZHMuaA0KPiA+IEBAIC0yOCwzICsyOCw1
IEBAIGludCBkb19zYW5pdGl6ZShpbnQgbmFyZ3MsIGNoYXIgKiphcmd2KTsgIGludA0KPiA+IGRv
X3N0YXR1c19nZXQoaW50IG5hcmdzLCBjaGFyICoqYXJndik7ICBpbnQgZG9fZW5oX2FyZWFfc2V0
KGludCBuYXJncywNCj4gPiBjaGFyICoqYXJndik7ICBpbnQgZG9fd3JpdGVfcmVsaWFiaWxpdHlf
c2V0KGludCBuYXJncywgY2hhciAqKmFyZ3YpOw0KPiA+ICtpbnQgZG9fZW1tYzUwX2ZmdShpbnQg
bmFyZ3MsIGNoYXIgKiphcmd2KTsNCj4gPiArDQo+ID4gLS0NCj4gPiAxLjcuNS40DQo+ID4NCj4g
Pg0KPiA+IEF2aSBTaGNoaXNsb3dza2kgfCBTdGFmZiBTb2Z0d2FyZSBFbmdpbmVlciwgTUNTIEVt
YmVkZGVkICB8IFNhbkRpc2sgfA0KPiA+ICs5NzIuMDkuNzYzLTI2NjZ8IHd3dy5zYW5kaXNrLmNv
bQ0KPiA+DQo+IA0KPiBjaGVlcnMsDQo+IGdyYW50DQoNCgoK


--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Alex Lemberg March 5, 2014, 4:35 p.m. UTC | #4

Hi Kees,

Thanks for your comment.
Please see our response inline.


> -----Original Message-----

> From: [e-mail] [mailto:[e-mail]] On Behalf Of

> Kees Cook

> Sent: Saturday, March 01, 2014 1:21 AM

> To: Grant Grundler

> Cc: Avi Shchislowski; [e-mail]; [e-mail]; Alex

> Lemberg; Gwendal Grignou; Puthikorn Voravootivat

> Subject: Re: [RFC PATCH 1/1] mmc-utils: Support-sending-eMMC-5.0-FFU

> 

> On Fri, Feb 28, 2014 at 2:39 PM, Grant Grundler <[e-mail]>

> wrote:

> > Avi,

> > Thanks for posting these - I look forward to seeing this functionality

> > available in mmc-utils (and kernel as needed).

> >

> > Comments as usual inline.

> >

> > I've added Gwendal/Kees to CC to comment on security issues of this

> > proposal. See notes below.

> >

> >

> > On Sun, Feb 9, 2014 at 1:08 AM, Avi Shchislowski

> > <[e-mail]> wrote:

> >>   The mmc-utils was modified to invoke eMMC5.0 Field Firmware Update

> (FFU) process in mmc driver

> >>   New command was add: "do_emmc50_ffu".

> >>

> >>   This patch depends on patch  mmc: Support-FFU-for-eMMC-v5.0

> >> Committed by Avi Shchislowski <[e-mail]>

> >>

> >>   FFU will be done in two steps. Two new IOCTL codes will be sent to the

> driver in order to operate FFU code:

> >>     1.  FFU_DWONLOAD_OP (sent in ffu_download_image() function)

> >

> > Any reason for the typo? DOWNLOAD maybe?

> > Shouldn't that be MMC_FFU_DOWNLOAD_OP to match the proposed

> kernel definition?

> >

> >>         2.  FFU_INSTALL_OP (sent in ffu_install() function)

> >

> > Ditto: MMC_FFU_INSTALL_OP

> >

> >>

> >>

> >> Signed-off-by: Avi Shchislowski <[e-mail]>

> >> Signed-off-by: Alex Lemberg <[e-mail]>

> >>

> >> diff --git a/mmc.c b/mmc.c

> >> index 926e92f..a01852d 100644

> >> --- a/mmc.c

> >> +++ b/mmc.c

> >> @@ -36,9 +36,9 @@ struct Command {

> >>                                    if >= 0, number of arguments,

> >>                                    if < 0, _minimum_ number of arguments */

> >>         char    *verb;          /* verb */

> >> -       char    *help;          /* help lines; from the 2nd line onward they

> >> +       char    *help;          /* help lines; from the 2nd line onward they

> >>                                     are automatically indented */

> >> -        char    *adv_help;      /* advanced help message; from the 2nd line

> >> +        char    *adv_help;      /* advanced help message; from the 2nd line

> >

> > Sorry, it's not obvious what changed here. Why is this included?

> >

> >>                                     onward they are automatically

> >> indented */

> >>

> >>         /* the following fields are run-time filled by the program */ @@ -

> 110,6 +110,11 @@ static struct Command commands[] = {

> >>                 "Send Sanitize command to the <device>.\nThis will delete the

> unmapped memory region of the device.",

> >>           NULL

> >>         },

> >> +       { do_emmc50_ffu, -2,

> >> +       "emmc50 ffu", "<image path> <device>\n"

> >> +         "run eMMC 5.0 Field firmware update.\n.",

> >

> > Nit: This isn't "run". It's "download firmware to eMMC 5.0 compliant

> device".

> >

> >> +         NULL

> >> +       },

> >>         { 0, 0, 0, 0 }

> >>  };

> >>

> >> @@ -362,7 +367,7 @@ static int parse_args(int argc, char **argv,

> >>                         matchcmd->verb, matchcmd->nargs);

> >>                         return -2;

> >>         }

> >> -

> >> +

> >

> > I'm going to ignore white space mangle on this patch and assume you'll

> > ask if you need help using gmail to send patches using git send-email.

> >

> > But the above isn't white space mangle caused by email - it's part of

> > this patch and I'm not seeing a difference in this <REDACTED> gmail

> > editor.

> >

> >>          if (prepare_args( nargs_, args_, prgname, matchcmd )){

> >>                  fprintf(stderr, "ERROR: not enough memory\\n");

> >>                 return -20;

> >> diff --git a/mmc.h b/mmc.h

> >> index 9871d62..3be6db0 100644

> >> --- a/mmc.h

> >> +++ b/mmc.h

> >> @@ -80,6 +80,14 @@

> >>  #define BKOPS_ENABLE   (1<<0)

> >>

> >>  /*

> >> + * sector size

> >> +*/

> >> +#define CARD_BLOCK_SIZE        512

> >

> > sector size is advertised by the device. It could be either 512 or 4K bytes.

> No?

> >

> > "7.4.17 NUMBER_OF_FW_SECTORS_CORRECTLY_PROGRAMMED [305-302]

> >

> > The value is in terms of 512 Bytes or in multiple of eight 512Bytes

> > sectors (4KBytes) depending on the value of the DATA_SECTOR_SIZE field."

> >

> > I don't think this should be hard coded to 512. And a few places I see

> > hard coded with "<< 9" will likely need to take this into account.

> >

> >

> >> +

> >> +#define FFU_DWONLOAD_OP        302

> >> +#define FFU_INSTALL_OP 303

> >

> > These should match kernel definitions (complete name and value).

> >

> >> +

> >> +/*

> >>   * EXT_CSD field definitions

> >>   */

> >>  #define EXT_CSD_HPI_SUPP               (1<<0)

> >> diff --git a/mmc_cmds.c b/mmc_cmds.c

> >> index b8afa74..24c4a6b 100644

> >> --- a/mmc_cmds.c

> >> +++ b/mmc_cmds.c

> >> @@ -1163,3 +1163,112 @@ int do_sanitize(int nargs, char **argv)

> >>

> >>  }

> >>

> >> +static int ffu_download_image(int fw_fd, int mmc_fd) {

> >> +       int ret = 0;

> >> +       struct mmc_ioc_cmd mmc_ioc_cmd;

> >> +       char data_buff[MMC_IOC_MAX_BYTES];

> >> +       int file_size;

> >

> > This should be off_t type. See "man 2 lseek".

> >

> >> +       int size;

> >

> > This should be size_t type.  See "man 2 read".

> >

> >> +       int data_length;

> >

> > This should ssize_t type. See "man 2 read".

> >

> >> +

> >> +       memset(data_buff, 0, sizeof(data_buff));

> >> +       /* get file size */

> >> +       file_size = lseek(fw_fd, 0, SEEK_END);

> >

> > I'm wondering why lseek would be preferred over fstat().

> >

> >> +       if (file_size < 0) {

> >> +               ret =  -1;

> >> +               perror("seek file error \n");

> >> +               goto exit;

> >> +       }

> >> +

> >> +       lseek(fw_fd, 0, SEEK_SET);

> >> +       do {

> >> +               size = (file_size > MMC_IOC_MAX_BYTES) ?

> >> +                               MMC_IOC_MAX_BYTES : file_size;

> >> +               /* Read FW data from file */

> >> +               data_length = read(fw_fd, data_buff, size);

> >> +               if (data_length == -1) {

> >

> > Should this test data_length < size ?

> >

> >> +                       ret = -1;

> >> +                       goto exit;

> >> +               }

> >

> > Gwendal and Kees (CC'd) would prefer to send the file name to the

> > kernel as part of the ioctl and use existing udev mechanisms to

> > request the firmware.

> 

> Yes, please see Documentation/firmware_class/README for information on

> the kernel internals, but I would much prefer the kernel do all the loading,

> not userspace. The kernel driver can request the firmware

> contents:

> 

>          if(request_firmware(&fw_entry, $FIRMWARE, device) == 0)

>                 copy_fw_to_device(fw_entry->data, fw_entry->size);

>          release(fw_entry);

> 

> and then send it to the device. Doing this from userspace means there is no

> way to verify the firmware contents. Equally, the kernel should actively block

> the MMC_FFU_DOWNLOAD_OP op, since it should be considered a sensitive

> operation.


Indeed this mechanism allows to download FW file directly to the driver, and not using IOCTL for this.

But actually, eMMC5.0 spec does not requires any of FW file content to be verified by the host.
The FW file should  be downloaded entirely and verified by eMMC device internally.

Please let us know if you aware of other solutions, which are requires FW file verification in the host side.

In the way that we have implemented FW download routine in the driver, the FW download process is blocked (using claim_host()) anyway, 
and prevents interruptions of this process by other IO requests.

> 

> -Kees

> 

> >

> > This has some advantages for security which make it a lot harder to

> > "plant" the hacked firmware on devices. I'll let Gwendal and Kees

> > present the details of those ideas.

> >

> >> +               /* prepare and send ioctl */

> >> +               memset(&mmc_ioc_cmd, 0, sizeof(mmc_ioc_cmd));

> >> +               mmc_ioc_cmd.opcode =  FFU_DWONLOAD_OP;

> >> +               mmc_ioc_cmd.blksz = CARD_BLOCK_SIZE;

> >> +               mmc_ioc_cmd.blocks = data_length / mmc_ioc_cmd.blksz;

> >> +               mmc_ioc_cmd.arg =  0;

> >> +               mmc_ioc_cmd.flags = MMC_RSP_SPI_R1 | MMC_RSP_R1 |

> MMC_CMD_ADTC;

> >> +               mmc_ioc_cmd.write_flag = 1;

> >> +               mmc_ioc_cmd_set_data(mmc_ioc_cmd, data_buff);

> >> +               ret = ioctl(mmc_fd, MMC_IOC_CMD, &mmc_ioc_cmd);

> >> +               if (ret) {

> >> +                       perror("ioctl FW download");

> >> +                       goto exit;

> >> +               }

> >> +

> >> +               file_size = file_size - size;

> >> +               printf("firmware file loading, remaining:   %d\n", file_size);

> >> +       } while (file_size > 0);

> >> +

> >> +exit:

> >> +

> >> +       return ret;

> >> +}

> >> +

> >> +static int ffu_install(int mmc_fd)

> >> +{

> >> +       int ret;

> >> +       struct mmc_ioc_cmd mmc_ioc_cmd;

> >> +

> >> +       memset(&mmc_ioc_cmd, 0, sizeof(mmc_ioc_cmd));

> >> +       mmc_ioc_cmd.opcode = FFU_INSTALL_OP;

> >> +       mmc_ioc_cmd.blksz = CARD_BLOCK_SIZE;

> >> +       mmc_ioc_cmd.blocks = 0;

> >> +       mmc_ioc_cmd.arg =  0;

> >> +       mmc_ioc_cmd.flags = MMC_RSP_SPI_R1 | MMC_RSP_R1 |

> MMC_CMD_ADTC;

> >> +       mmc_ioc_cmd.write_flag = 0;

> >> +       ret = ioctl(mmc_fd, MMC_IOC_CMD, &mmc_ioc_cmd);

> >> +       if (ret)

> >> +               perror("ioctl install");

> >> +

> >> +       printf("ffu_install ret %d \n", ret);

> >> +       return ret;

> >> +}

> >> +

> >> +int do_emmc50_ffu(int nargs, char **argv) {

> >> +       int fd, fw_fd, ret;

> >> +       char *device;

> >> +

> >> +       CHECK(nargs != 3, "Usage: ffu <image path> </path/to/mmcblkX>

> \n",

> >> +                               exit(1));

> >> +

> >> +       device = argv[2];

> >> +       fd = open(device, O_RDWR);

> >> +       if (fd < 0) {

> >> +               perror("open");

> >> +               exit(1);

> >> +       }

> >> +

> >> +       /* open eMMC5.0 firmware image file */

> >> +       fw_fd = open(argv[1], O_RDONLY);

> >> +       if (fw_fd < 0) {

> >> +               perror("open eMMC5.0 firmware file");

> >> +               ret = -1;

> >

> > Don't want to return the errno value?

> >

> >> +               goto exit;

> >> +       }

> >> +

> >> +       ret = ffu_download_image(fw_fd, fd);

> >> +       if (ret)

> >> +               goto exit;

> >> +

> >> +       ret = ffu_install(fd);

> >> +       if (ret)

> >> +               goto exit;

> >> +

> >> +exit:

> >> +       close(fd);

> >> +       close(fw_fd);

> >> +

> >> +       return ret;

> >> +}

> >> diff --git a/mmc_cmds.h b/mmc_cmds.h

> >> index f06cc10..77a6cb8 100644

> >> --- a/mmc_cmds.h

> >> +++ b/mmc_cmds.h

> >> @@ -28,3 +28,5 @@ int do_sanitize(int nargs, char **argv);  int

> >> do_status_get(int nargs, char **argv);  int do_enh_area_set(int

> >> nargs, char **argv);  int do_write_reliability_set(int nargs, char

> >> **argv);

> >> +int do_emmc50_ffu(int nargs, char **argv);

> >> +

> >> --

> >> 1.7.5.4

> >>

> >>

> >> Avi Shchislowski | Staff Software Engineer, MCS Embedded  | SanDisk |

> >> +972.09.763-2666| www.sandisk.com

> >>

> >

> > cheers,

> > grant

> 

> 

> 

> --

> Kees Cook

> Chrome OS Security

Gwendal Grignou March 5, 2014, 5:55 p.m. UTC | #5

On Wed, Mar 5, 2014 at 8:35 AM, Alex Lemberg <[e-mail]> wrote:
> Hi Kees,
>
> Thanks for your comment.
> Please see our response inline.
>
>
>> -----Original Message-----
>> From: [e-mail] [mailto:[e-mail]] On Behalf Of
>> Kees Cook
>> Sent: Saturday, March 01, 2014 1:21 AM
>> To: Grant Grundler
>> Cc: Avi Shchislowski; [e-mail]; [e-mail]; Alex
>> Lemberg; Gwendal Grignou; Puthikorn Voravootivat
>> Subject: Re: [RFC PATCH 1/1] mmc-utils: Support-sending-eMMC-5.0-FFU
>>
>> On Fri, Feb 28, 2014 at 2:39 PM, Grant Grundler <[e-mail]>
>> wrote:
>> > Avi,
>> > Thanks for posting these - I look forward to seeing this functionality
>> > available in mmc-utils (and kernel as needed).
>> >
>> > Comments as usual inline.
>> >
>> > I've added Gwendal/Kees to CC to comment on security issues of this
>> > proposal. See notes below.
>> >
>> >
>> > On Sun, Feb 9, 2014 at 1:08 AM, Avi Shchislowski
>> > <[e-mail]> wrote:
>> >>   The mmc-utils was modified to invoke eMMC5.0 Field Firmware Update
>> (FFU) process in mmc driver
>> >>   New command was add: "do_emmc50_ffu".
>> >>
>> >>   This patch depends on patch  mmc: Support-FFU-for-eMMC-v5.0
>> >> Committed by Avi Shchislowski <[e-mail]>
>> >>
>> >>   FFU will be done in two steps. Two new IOCTL codes will be sent to the
>> driver in order to operate FFU code:
>> >>     1.  FFU_DWONLOAD_OP (sent in ffu_download_image() function)
>> >
>> > Any reason for the typo? DOWNLOAD maybe?
>> > Shouldn't that be MMC_FFU_DOWNLOAD_OP to match the proposed
>> kernel definition?
>> >
>> >>         2.  FFU_INSTALL_OP (sent in ffu_install() function)
>> >
>> > Ditto: MMC_FFU_INSTALL_OP
>> >
>> >>
>> >>
>> >> Signed-off-by: Avi Shchislowski <[e-mail]>
>> >> Signed-off-by: Alex Lemberg <[e-mail]>
>> >>
>> >> diff --git a/mmc.c b/mmc.c
>> >> index 926e92f..a01852d 100644
>> >> --- a/mmc.c
>> >> +++ b/mmc.c
>> >> @@ -36,9 +36,9 @@ struct Command {
>> >>                                    if >= 0, number of arguments,
>> >>                                    if < 0, _minimum_ number of arguments */
>> >>         char    *verb;          /* verb */
>> >> -       char    *help;          /* help lines; from the 2nd line onward they
>> >> +       char    *help;          /* help lines; from the 2nd line onward they
>> >>                                     are automatically indented */
>> >> -        char    *adv_help;      /* advanced help message; from the 2nd line
>> >> +        char    *adv_help;      /* advanced help message; from the 2nd line
>> >
>> > Sorry, it's not obvious what changed here. Why is this included?
>> >
>> >>                                     onward they are automatically
>> >> indented */
>> >>
>> >>         /* the following fields are run-time filled by the program */ @@ -
>> 110,6 +110,11 @@ static struct Command commands[] = {
>> >>                 "Send Sanitize command to the <device>.\nThis will delete the
>> unmapped memory region of the device.",
>> >>           NULL
>> >>         },
>> >> +       { do_emmc50_ffu, -2,
>> >> +       "emmc50 ffu", "<image path> <device>\n"
>> >> +         "run eMMC 5.0 Field firmware update.\n.",
>> >
>> > Nit: This isn't "run". It's "download firmware to eMMC 5.0 compliant
>> device".
>> >
>> >> +         NULL
>> >> +       },
>> >>         { 0, 0, 0, 0 }
>> >>  };
>> >>
>> >> @@ -362,7 +367,7 @@ static int parse_args(int argc, char **argv,
>> >>                         matchcmd->verb, matchcmd->nargs);
>> >>                         return -2;
>> >>         }
>> >> -
>> >> +
>> >
>> > I'm going to ignore white space mangle on this patch and assume you'll
>> > ask if you need help using gmail to send patches using git send-email.
>> >
>> > But the above isn't white space mangle caused by email - it's part of
>> > this patch and I'm not seeing a difference in this <REDACTED> gmail
>> > editor.
>> >
>> >>          if (prepare_args( nargs_, args_, prgname, matchcmd )){
>> >>                  fprintf(stderr, "ERROR: not enough memory\\n");
>> >>                 return -20;
>> >> diff --git a/mmc.h b/mmc.h
>> >> index 9871d62..3be6db0 100644
>> >> --- a/mmc.h
>> >> +++ b/mmc.h
>> >> @@ -80,6 +80,14 @@
>> >>  #define BKOPS_ENABLE   (1<<0)
>> >>
>> >>  /*
>> >> + * sector size
>> >> +*/
>> >> +#define CARD_BLOCK_SIZE        512
>> >
>> > sector size is advertised by the device. It could be either 512 or 4K bytes.
>> No?
>> >
>> > "7.4.17 NUMBER_OF_FW_SECTORS_CORRECTLY_PROGRAMMED [305-302]
>> >
>> > The value is in terms of 512 Bytes or in multiple of eight 512Bytes
>> > sectors (4KBytes) depending on the value of the DATA_SECTOR_SIZE field."
>> >
>> > I don't think this should be hard coded to 512. And a few places I see
>> > hard coded with "<< 9" will likely need to take this into account.
>> >
>> >
>> >> +
>> >> +#define FFU_DWONLOAD_OP        302
>> >> +#define FFU_INSTALL_OP 303
>> >
>> > These should match kernel definitions (complete name and value).
>> >
>> >> +
>> >> +/*
>> >>   * EXT_CSD field definitions
>> >>   */
>> >>  #define EXT_CSD_HPI_SUPP               (1<<0)
>> >> diff --git a/mmc_cmds.c b/mmc_cmds.c
>> >> index b8afa74..24c4a6b 100644
>> >> --- a/mmc_cmds.c
>> >> +++ b/mmc_cmds.c
>> >> @@ -1163,3 +1163,112 @@ int do_sanitize(int nargs, char **argv)
>> >>
>> >>  }
>> >>
>> >> +static int ffu_download_image(int fw_fd, int mmc_fd) {
>> >> +       int ret = 0;
>> >> +       struct mmc_ioc_cmd mmc_ioc_cmd;
>> >> +       char data_buff[MMC_IOC_MAX_BYTES];
>> >> +       int file_size;
>> >
>> > This should be off_t type. See "man 2 lseek".
>> >
>> >> +       int size;
>> >
>> > This should be size_t type.  See "man 2 read".
>> >
>> >> +       int data_length;
>> >
>> > This should ssize_t type. See "man 2 read".
>> >
>> >> +
>> >> +       memset(data_buff, 0, sizeof(data_buff));
>> >> +       /* get file size */
>> >> +       file_size = lseek(fw_fd, 0, SEEK_END);
>> >
>> > I'm wondering why lseek would be preferred over fstat().
>> >
>> >> +       if (file_size < 0) {
>> >> +               ret =  -1;
>> >> +               perror("seek file error \n");
>> >> +               goto exit;
>> >> +       }
>> >> +
>> >> +       lseek(fw_fd, 0, SEEK_SET);
>> >> +       do {
>> >> +               size = (file_size > MMC_IOC_MAX_BYTES) ?
>> >> +                               MMC_IOC_MAX_BYTES : file_size;
>> >> +               /* Read FW data from file */
>> >> +               data_length = read(fw_fd, data_buff, size);
>> >> +               if (data_length == -1) {
>> >
>> > Should this test data_length < size ?
>> >
>> >> +                       ret = -1;
>> >> +                       goto exit;
>> >> +               }
>> >
>> > Gwendal and Kees (CC'd) would prefer to send the file name to the
>> > kernel as part of the ioctl and use existing udev mechanisms to
>> > request the firmware.
>>
>> Yes, please see Documentation/firmware_class/README for information on
>> the kernel internals, but I would much prefer the kernel do all the loading,
>> not userspace. The kernel driver can request the firmware
>> contents:
>>
>>          if(request_firmware(&fw_entry, $FIRMWARE, device) == 0)
>>                 copy_fw_to_device(fw_entry->data, fw_entry->size);
>>          release(fw_entry);
>>
>> and then send it to the device. Doing this from userspace means there is no
>> way to verify the firmware contents. Equally, the kernel should actively block
>> the MMC_FFU_DOWNLOAD_OP op, since it should be considered a sensitive
>> operation.
>
> Indeed this mechanism allows to download FW file directly to the driver, and not using IOCTL for this.
>
> But actually, eMMC5.0 spec does not requires any of FW file content to be verified by the host.
> The FW file should  be downloaded entirely and verified by eMMC device internally.

The point of using firmware_request() is the firmware image is not
sent within the IOCTL itself but provided by a well known daemon, in
this case udevd.
We can make udevd the gate keeper of firmware images [not only eMMC
devices, but wifi and 3g modems as well] and ensure that only approved
and well-known images are sent to the devices.

>
> Please let us know if you aware of other solutions, which are requires FW file verification in the host side.
>
> In the way that we have implemented FW download routine in the driver, the FW download process is blocked (using claim_host()) anyway,
> and prevents interruptions of this process by other IO requests.
>
>>
>> -Kees
>>
>> >
>> > This has some advantages for security which make it a lot harder to
>> > "plant" the hacked firmware on devices. I'll let Gwendal and Kees
>> > present the details of those ideas.
>> >
>> >> +               /* prepare and send ioctl */
>> >> +               memset(&mmc_ioc_cmd, 0, sizeof(mmc_ioc_cmd));
>> >> +               mmc_ioc_cmd.opcode =  FFU_DWONLOAD_OP;
>> >> +               mmc_ioc_cmd.blksz = CARD_BLOCK_SIZE;
>> >> +               mmc_ioc_cmd.blocks = data_length / mmc_ioc_cmd.blksz;
>> >> +               mmc_ioc_cmd.arg =  0;
>> >> +               mmc_ioc_cmd.flags = MMC_RSP_SPI_R1 | MMC_RSP_R1 |
>> MMC_CMD_ADTC;
>> >> +               mmc_ioc_cmd.write_flag = 1;
>> >> +               mmc_ioc_cmd_set_data(mmc_ioc_cmd, data_buff);
>> >> +               ret = ioctl(mmc_fd, MMC_IOC_CMD, &mmc_ioc_cmd);
>> >> +               if (ret) {
>> >> +                       perror("ioctl FW download");
>> >> +                       goto exit;
>> >> +               }
>> >> +
>> >> +               file_size = file_size - size;
>> >> +               printf("firmware file loading, remaining:   %d\n", file_size);
>> >> +       } while (file_size > 0);
>> >> +
>> >> +exit:
>> >> +
>> >> +       return ret;
>> >> +}
>> >> +
>> >> +static int ffu_install(int mmc_fd)
>> >> +{
>> >> +       int ret;
>> >> +       struct mmc_ioc_cmd mmc_ioc_cmd;
>> >> +
>> >> +       memset(&mmc_ioc_cmd, 0, sizeof(mmc_ioc_cmd));
>> >> +       mmc_ioc_cmd.opcode = FFU_INSTALL_OP;
>> >> +       mmc_ioc_cmd.blksz = CARD_BLOCK_SIZE;
>> >> +       mmc_ioc_cmd.blocks = 0;
>> >> +       mmc_ioc_cmd.arg =  0;
>> >> +       mmc_ioc_cmd.flags = MMC_RSP_SPI_R1 | MMC_RSP_R1 |
>> MMC_CMD_ADTC;
>> >> +       mmc_ioc_cmd.write_flag = 0;
>> >> +       ret = ioctl(mmc_fd, MMC_IOC_CMD, &mmc_ioc_cmd);
>> >> +       if (ret)
>> >> +               perror("ioctl install");
>> >> +
>> >> +       printf("ffu_install ret %d \n", ret);
>> >> +       return ret;
>> >> +}
>> >> +
>> >> +int do_emmc50_ffu(int nargs, char **argv) {
>> >> +       int fd, fw_fd, ret;
>> >> +       char *device;
>> >> +
>> >> +       CHECK(nargs != 3, "Usage: ffu <image path> </path/to/mmcblkX>
>> \n",
>> >> +                               exit(1));
>> >> +
>> >> +       device = argv[2];
>> >> +       fd = open(device, O_RDWR);
>> >> +       if (fd < 0) {
>> >> +               perror("open");
>> >> +               exit(1);
>> >> +       }
>> >> +
>> >> +       /* open eMMC5.0 firmware image file */
>> >> +       fw_fd = open(argv[1], O_RDONLY);
>> >> +       if (fw_fd < 0) {
>> >> +               perror("open eMMC5.0 firmware file");
>> >> +               ret = -1;
>> >
>> > Don't want to return the errno value?
>> >
>> >> +               goto exit;
>> >> +       }
>> >> +
>> >> +       ret = ffu_download_image(fw_fd, fd);
>> >> +       if (ret)
>> >> +               goto exit;
>> >> +
>> >> +       ret = ffu_install(fd);
>> >> +       if (ret)
>> >> +               goto exit;
>> >> +
>> >> +exit:
>> >> +       close(fd);
>> >> +       close(fw_fd);
>> >> +
>> >> +       return ret;
>> >> +}
>> >> diff --git a/mmc_cmds.h b/mmc_cmds.h
>> >> index f06cc10..77a6cb8 100644
>> >> --- a/mmc_cmds.h
>> >> +++ b/mmc_cmds.h
>> >> @@ -28,3 +28,5 @@ int do_sanitize(int nargs, char **argv);  int
>> >> do_status_get(int nargs, char **argv);  int do_enh_area_set(int
>> >> nargs, char **argv);  int do_write_reliability_set(int nargs, char
>> >> **argv);
>> >> +int do_emmc50_ffu(int nargs, char **argv);
>> >> +
>> >> --
>> >> 1.7.5.4
>> >>
>> >>
>> >> Avi Shchislowski | Staff Software Engineer, MCS Embedded  | SanDisk |
>> >> +972.09.763-2666| www.sandisk.com
>> >>
>> >
>> > cheers,
>> > grant
>>
>>
>>
>> --
>> Kees Cook
>> Chrome OS Security
>
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Kees Cook March 5, 2014, 9:58 p.m. UTC | #6

On Wed, Mar 5, 2014 at 9:52 AM, Gwendal Grignou <[e-mail]> wrote:
>
>
>
> On Wed, Mar 5, 2014 at 8:35 AM, Alex Lemberg <[e-mail]>
> wrote:
>>
>> Hi Kees,
>>
>> Thanks for your comment.
>> Please see our response inline.
>>
>>
>> > -----Original Message-----
>> > From: [e-mail] [mailto:[e-mail]] On Behalf Of
>> > Kees Cook
>> > Sent: Saturday, March 01, 2014 1:21 AM
>> > To: Grant Grundler
>> > Cc: Avi Shchislowski; [e-mail]; [e-mail]; Alex
>> > Lemberg; Gwendal Grignou; Puthikorn Voravootivat
>> > Subject: Re: [RFC PATCH 1/1] mmc-utils: Support-sending-eMMC-5.0-FFU
>> >
>> > On Fri, Feb 28, 2014 at 2:39 PM, Grant Grundler <[e-mail]>
>> > wrote:
>> > > Avi,
>> > > Thanks for posting these - I look forward to seeing this functionality
>> > > available in mmc-utils (and kernel as needed).
>> > >
>> > > Comments as usual inline.
>> > >
>> > > I've added Gwendal/Kees to CC to comment on security issues of this
>> > > proposal. See notes below.
>> > >
>> > >
>> > > On Sun, Feb 9, 2014 at 1:08 AM, Avi Shchislowski
>> > > <[e-mail]> wrote:
>> > >>   The mmc-utils was modified to invoke eMMC5.0 Field Firmware Update
>> > (FFU) process in mmc driver
>> > >>   New command was add: "do_emmc50_ffu".
>> > >>
>> > >>   This patch depends on patch  mmc: Support-FFU-for-eMMC-v5.0
>> > >> Committed by Avi Shchislowski <[e-mail]>
>> > >>
>> > >>   FFU will be done in two steps. Two new IOCTL codes will be sent to
>> > >> the
>> > driver in order to operate FFU code:
>> > >>     1.  FFU_DWONLOAD_OP (sent in ffu_download_image() function)
>> > >
>> > > Any reason for the typo? DOWNLOAD maybe?
>> > > Shouldn't that be MMC_FFU_DOWNLOAD_OP to match the proposed
>> > kernel definition?
>> > >
>> > >>         2.  FFU_INSTALL_OP (sent in ffu_install() function)
>> > >
>> > > Ditto: MMC_FFU_INSTALL_OP
>> > >
>> > >>
>> > >>
>> > >> Signed-off-by: Avi Shchislowski <[e-mail]>
>> > >> Signed-off-by: Alex Lemberg <[e-mail]>
>> > >>
>> > >> diff --git a/mmc.c b/mmc.c
>> > >> index 926e92f..a01852d 100644
>> > >> --- a/mmc.c
>> > >> +++ b/mmc.c
>> > >> @@ -36,9 +36,9 @@ struct Command {
>> > >>                                    if >= 0, number of arguments,
>> > >>                                    if < 0, _minimum_ number of
>> > >> arguments */
>> > >>         char    *verb;          /* verb */
>> > >> -       char    *help;          /* help lines; from the 2nd line
>> > >> onward they
>> > >> +       char    *help;          /* help lines; from the 2nd line
>> > >> onward they
>> > >>                                     are automatically indented */
>> > >> -        char    *adv_help;      /* advanced help message; from the
>> > >> 2nd line
>> > >> +        char    *adv_help;      /* advanced help message; from the
>> > >> 2nd line
>> > >
>> > > Sorry, it's not obvious what changed here. Why is this included?
>> > >
>> > >>                                     onward they are automatically
>> > >> indented */
>> > >>
>> > >>         /* the following fields are run-time filled by the program */
>> > >> @@ -
>> > 110,6 +110,11 @@ static struct Command commands[] = {
>> > >>                 "Send Sanitize command to the <device>.\nThis will
>> > >> delete the
>> > unmapped memory region of the device.",
>> > >>           NULL
>> > >>         },
>> > >> +       { do_emmc50_ffu, -2,
>> > >> +       "emmc50 ffu", "<image path> <device>\n"
>> > >> +         "run eMMC 5.0 Field firmware update.\n.",
>> > >
>> > > Nit: This isn't "run". It's "download firmware to eMMC 5.0 compliant
>> > device".
>> > >
>> > >> +         NULL
>> > >> +       },
>> > >>         { 0, 0, 0, 0 }
>> > >>  };
>> > >>
>> > >> @@ -362,7 +367,7 @@ static int parse_args(int argc, char **argv,
>> > >>                         matchcmd->verb, matchcmd->nargs);
>> > >>                         return -2;
>> > >>         }
>> > >> -
>> > >> +
>> > >
>> > > I'm going to ignore white space mangle on this patch and assume you'll
>> > > ask if you need help using gmail to send patches using git send-email.
>> > >
>> > > But the above isn't white space mangle caused by email - it's part of
>> > > this patch and I'm not seeing a difference in this <REDACTED> gmail
>> > > editor.
>> > >
>> > >>          if (prepare_args( nargs_, args_, prgname, matchcmd )){
>> > >>                  fprintf(stderr, "ERROR: not enough memory\\n");
>> > >>                 return -20;
>> > >> diff --git a/mmc.h b/mmc.h
>> > >> index 9871d62..3be6db0 100644
>> > >> --- a/mmc.h
>> > >> +++ b/mmc.h
>> > >> @@ -80,6 +80,14 @@
>> > >>  #define BKOPS_ENABLE   (1<<0)
>> > >>
>> > >>  /*
>> > >> + * sector size
>> > >> +*/
>> > >> +#define CARD_BLOCK_SIZE        512
>> > >
>> > > sector size is advertised by the device. It could be either 512 or 4K
>> > > bytes.
>> > No?
>> > >
>> > > "7.4.17 NUMBER_OF_FW_SECTORS_CORRECTLY_PROGRAMMED [305-302]
>> > >
>> > > The value is in terms of 512 Bytes or in multiple of eight 512Bytes
>> > > sectors (4KBytes) depending on the value of the DATA_SECTOR_SIZE
>> > > field."
>> > >
>> > > I don't think this should be hard coded to 512. And a few places I see
>> > > hard coded with "<< 9" will likely need to take this into account.
>> > >
>> > >
>> > >> +
>> > >> +#define FFU_DWONLOAD_OP        302
>> > >> +#define FFU_INSTALL_OP 303
>> > >
>> > > These should match kernel definitions (complete name and value).
>> > >
>> > >> +
>> > >> +/*
>> > >>   * EXT_CSD field definitions
>> > >>   */
>> > >>  #define EXT_CSD_HPI_SUPP               (1<<0)
>> > >> diff --git a/mmc_cmds.c b/mmc_cmds.c
>> > >> index b8afa74..24c4a6b 100644
>> > >> --- a/mmc_cmds.c
>> > >> +++ b/mmc_cmds.c
>> > >> @@ -1163,3 +1163,112 @@ int do_sanitize(int nargs, char **argv)
>> > >>
>> > >>  }
>> > >>
>> > >> +static int ffu_download_image(int fw_fd, int mmc_fd) {
>> > >> +       int ret = 0;
>> > >> +       struct mmc_ioc_cmd mmc_ioc_cmd;
>> > >> +       char data_buff[MMC_IOC_MAX_BYTES];
>> > >> +       int file_size;
>> > >
>> > > This should be off_t type. See "man 2 lseek".
>> > >
>> > >> +       int size;
>> > >
>> > > This should be size_t type.  See "man 2 read".
>> > >
>> > >> +       int data_length;
>> > >
>> > > This should ssize_t type. See "man 2 read".
>> > >
>> > >> +
>> > >> +       memset(data_buff, 0, sizeof(data_buff));
>> > >> +       /* get file size */
>> > >> +       file_size = lseek(fw_fd, 0, SEEK_END);
>> > >
>> > > I'm wondering why lseek would be preferred over fstat().
>> > >
>> > >> +       if (file_size < 0) {
>> > >> +               ret =  -1;
>> > >> +               perror("seek file error \n");
>> > >> +               goto exit;
>> > >> +       }
>> > >> +
>> > >> +       lseek(fw_fd, 0, SEEK_SET);
>> > >> +       do {
>> > >> +               size = (file_size > MMC_IOC_MAX_BYTES) ?
>> > >> +                               MMC_IOC_MAX_BYTES : file_size;
>> > >> +               /* Read FW data from file */
>> > >> +               data_length = read(fw_fd, data_buff, size);
>> > >> +               if (data_length == -1) {
>> > >
>> > > Should this test data_length < size ?
>> > >
>> > >> +                       ret = -1;
>> > >> +                       goto exit;
>> > >> +               }
>> > >
>> > > Gwendal and Kees (CC'd) would prefer to send the file name to the
>> > > kernel as part of the ioctl and use existing udev mechanisms to
>> > > request the firmware.
>> >
>> > Yes, please see Documentation/firmware_class/README for information on
>> > the kernel internals, but I would much prefer the kernel do all the
>> > loading,
>> > not userspace. The kernel driver can request the firmware
>> > contents:
>> >
>> >          if(request_firmware(&fw_entry, $FIRMWARE, device) == 0)
>> >                 copy_fw_to_device(fw_entry->data, fw_entry->size);
>> >          release(fw_entry);
>> >
>> > and then send it to the device. Doing this from userspace means there is
>> > no
>> > way to verify the firmware contents. Equally, the kernel should actively
>> > block
>> > the MMC_FFU_DOWNLOAD_OP op, since it should be considered a sensitive
>> > operation.
>>
>> Indeed this mechanism allows to download FW file directly to the driver,
>> and not using IOCTL for this.
>>
>> But actually, eMMC5.0 spec does not requires any of FW file content to be
>> verified by the host.
>> The FW file should  be downloaded entirely and verified by eMMC device
>> internally.
>
>
> The point of using firmware_request() is the firmware image is not sent
> within the IOCTL itself but provided by a well known daemon, in this case
> udevd.
> We can make udevd the gate keeper of firmware images [not only eMMC devices,
> but wifi and 3g modems as well] and ensure that only approved and well-known
> images are sent to the devices.

Not only udevd, but the kernel itself -- the kernel too. The goal is
to make the kernel the trusted element, and to not trust any portion
of userspace that can't be cryptographically verified.

-Kees

>
> Gwendal.
>>
>>
>> Please let us know if you aware of other solutions, which are requires FW
>> file verification in the host side.
>>
>> In the way that we have implemented FW download routine in the driver, the
>> FW download process is blocked (using claim_host()) anyway,
>> and prevents interruptions of this process by other IO requests.
>>
>> >
>> > -Kees
>> >
>> > >
>> > > This has some advantages for security which make it a lot harder to
>> > > "plant" the hacked firmware on devices. I'll let Gwendal and Kees
>> > > present the details of those ideas.
>> > >
>> > >> +               /* prepare and send ioctl */
>> > >> +               memset(&mmc_ioc_cmd, 0, sizeof(mmc_ioc_cmd));
>> > >> +               mmc_ioc_cmd.opcode =  FFU_DWONLOAD_OP;
>> > >> +               mmc_ioc_cmd.blksz = CARD_BLOCK_SIZE;
>> > >> +               mmc_ioc_cmd.blocks = data_length / mmc_ioc_cmd.blksz;
>> > >> +               mmc_ioc_cmd.arg =  0;
>> > >> +               mmc_ioc_cmd.flags = MMC_RSP_SPI_R1 | MMC_RSP_R1 |
>> > MMC_CMD_ADTC;
>> > >> +               mmc_ioc_cmd.write_flag = 1;
>> > >> +               mmc_ioc_cmd_set_data(mmc_ioc_cmd, data_buff);
>> > >> +               ret = ioctl(mmc_fd, MMC_IOC_CMD, &mmc_ioc_cmd);
>> > >> +               if (ret) {
>> > >> +                       perror("ioctl FW download");
>> > >> +                       goto exit;
>> > >> +               }
>> > >> +
>> > >> +               file_size = file_size - size;
>> > >> +               printf("firmware file loading, remaining:   %d\n",
>> > >> file_size);
>> > >> +       } while (file_size > 0);
>> > >> +
>> > >> +exit:
>> > >> +
>> > >> +       return ret;
>> > >> +}
>> > >> +
>> > >> +static int ffu_install(int mmc_fd)
>> > >> +{
>> > >> +       int ret;
>> > >> +       struct mmc_ioc_cmd mmc_ioc_cmd;
>> > >> +
>> > >> +       memset(&mmc_ioc_cmd, 0, sizeof(mmc_ioc_cmd));
>> > >> +       mmc_ioc_cmd.opcode = FFU_INSTALL_OP;
>> > >> +       mmc_ioc_cmd.blksz = CARD_BLOCK_SIZE;
>> > >> +       mmc_ioc_cmd.blocks = 0;
>> > >> +       mmc_ioc_cmd.arg =  0;
>> > >> +       mmc_ioc_cmd.flags = MMC_RSP_SPI_R1 | MMC_RSP_R1 |
>> > MMC_CMD_ADTC;
>> > >> +       mmc_ioc_cmd.write_flag = 0;
>> > >> +       ret = ioctl(mmc_fd, MMC_IOC_CMD, &mmc_ioc_cmd);
>> > >> +       if (ret)
>> > >> +               perror("ioctl install");
>> > >> +
>> > >> +       printf("ffu_install ret %d \n", ret);
>> > >> +       return ret;
>> > >> +}
>> > >> +
>> > >> +int do_emmc50_ffu(int nargs, char **argv) {
>> > >> +       int fd, fw_fd, ret;
>> > >> +       char *device;
>> > >> +
>> > >> +       CHECK(nargs != 3, "Usage: ffu <image path> </path/to/mmcblkX>
>> > \n",
>> > >> +                               exit(1));
>> > >> +
>> > >> +       device = argv[2];
>> > >> +       fd = open(device, O_RDWR);
>> > >> +       if (fd < 0) {
>> > >> +               perror("open");
>> > >> +               exit(1);
>> > >> +       }
>> > >> +
>> > >> +       /* open eMMC5.0 firmware image file */
>> > >> +       fw_fd = open(argv[1], O_RDONLY);
>> > >> +       if (fw_fd < 0) {
>> > >> +               perror("open eMMC5.0 firmware file");
>> > >> +               ret = -1;
>> > >
>> > > Don't want to return the errno value?
>> > >
>> > >> +               goto exit;
>> > >> +       }
>> > >> +
>> > >> +       ret = ffu_download_image(fw_fd, fd);
>> > >> +       if (ret)
>> > >> +               goto exit;
>> > >> +
>> > >> +       ret = ffu_install(fd);
>> > >> +       if (ret)
>> > >> +               goto exit;
>> > >> +
>> > >> +exit:
>> > >> +       close(fd);
>> > >> +       close(fw_fd);
>> > >> +
>> > >> +       return ret;
>> > >> +}
>> > >> diff --git a/mmc_cmds.h b/mmc_cmds.h
>> > >> index f06cc10..77a6cb8 100644
>> > >> --- a/mmc_cmds.h
>> > >> +++ b/mmc_cmds.h
>> > >> @@ -28,3 +28,5 @@ int do_sanitize(int nargs, char **argv);  int
>> > >> do_status_get(int nargs, char **argv);  int do_enh_area_set(int
>> > >> nargs, char **argv);  int do_write_reliability_set(int nargs, char
>> > >> **argv);
>> > >> +int do_emmc50_ffu(int nargs, char **argv);
>> > >> +
>> > >> --
>> > >> 1.7.5.4
>> > >>
>> > >>
>> > >> Avi Shchislowski | Staff Software Engineer, MCS Embedded  | SanDisk |
>> > >> +972.09.763-2666| www.sandisk.com
>> > >>
>> > >
>> > > cheers,
>> > > grant
>> >
>> >
>> >
>> > --
>> > Kees Cook
>> > Chrome OS Security
>>
>>
>>
>

Alex Lemberg March 10, 2014, 8:55 a.m. UTC | #7

Hi Kees and Gwendal,

Please see inline.

Thanks,
Alex

> -----Original Message-----

> From: [e-mail] [mailto:[e-mail]] On Behalf Of

> Kees Cook

> Sent: Wednesday, March 05, 2014 11:59 PM

> To: Gwendal Grignou

> Cc: Alex Lemberg; Grant Grundler; Avi Shchislowski; [e-mail]; linux-

> [e-mail]; Puthikorn Voravootivat

> Subject: Re: [RFC PATCH 1/1] mmc-utils: Support-sending-eMMC-5.0-FFU

> 

> On Wed, Mar 5, 2014 at 9:52 AM, Gwendal Grignou

> <[e-mail]> wrote:

> >

> >

> >

> > On Wed, Mar 5, 2014 at 8:35 AM, Alex Lemberg

> > <[e-mail]>

> > wrote:

> >>

> >> Hi Kees,

> >>

> >> Thanks for your comment.

> >> Please see our response inline.

> >>

> >>

> >> > -----Original Message-----

> >> > From: [e-mail] [mailto:[e-mail]] On

> Behalf Of

> >> > Kees Cook

> >> > Sent: Saturday, March 01, 2014 1:21 AM

> >> > To: Grant Grundler

> >> > Cc: Avi Shchislowski; [e-mail]; [e-mail];

> >> > Alex Lemberg; Gwendal Grignou; Puthikorn Voravootivat

> >> > Subject: Re: [RFC PATCH 1/1] mmc-utils:

> >> > Support-sending-eMMC-5.0-FFU

> >> >

> >> > On Fri, Feb 28, 2014 at 2:39 PM, Grant Grundler

> >> > <[e-mail]>

> >> > wrote:

> >> > > Avi,

> >> > > Thanks for posting these - I look forward to seeing this

> >> > > functionality available in mmc-utils (and kernel as needed).

> >> > >

> >> > > Comments as usual inline.

> >> > >

> >> > > I've added Gwendal/Kees to CC to comment on security issues of

> >> > > this proposal. See notes below.

> >> > >

> >> > >

> >> > > On Sun, Feb 9, 2014 at 1:08 AM, Avi Shchislowski

> >> > > <[e-mail]> wrote:

> >> > >>   The mmc-utils was modified to invoke eMMC5.0 Field Firmware

> >> > >> Update

> >> > (FFU) process in mmc driver

> >> > >>   New command was add: "do_emmc50_ffu".

> >> > >>

> >> > >>   This patch depends on patch  mmc: Support-FFU-for-eMMC-v5.0

> >> > >> Committed by Avi Shchislowski <[e-mail]>

> >> > >>

> >> > >>   FFU will be done in two steps. Two new IOCTL codes will be

> >> > >> sent to the

> >> > driver in order to operate FFU code:

> >> > >>     1.  FFU_DWONLOAD_OP (sent in ffu_download_image() function)

> >> > >

> >> > > Any reason for the typo? DOWNLOAD maybe?

> >> > > Shouldn't that be MMC_FFU_DOWNLOAD_OP to match the proposed

> >> > kernel definition?

> >> > >

> >> > >>         2.  FFU_INSTALL_OP (sent in ffu_install() function)

> >> > >

> >> > > Ditto: MMC_FFU_INSTALL_OP

> >> > >

> >> > >>

> >> > >>

> >> > >> Signed-off-by: Avi Shchislowski <[e-mail]>

> >> > >> Signed-off-by: Alex Lemberg <[e-mail]>

> >> > >>

> >> > >> diff --git a/mmc.c b/mmc.c

> >> > >> index 926e92f..a01852d 100644

> >> > >> --- a/mmc.c

> >> > >> +++ b/mmc.c

> >> > >> @@ -36,9 +36,9 @@ struct Command {

> >> > >>                                    if >= 0, number of arguments,

> >> > >>                                    if < 0, _minimum_ number of

> >> > >> arguments */

> >> > >>         char    *verb;          /* verb */

> >> > >> -       char    *help;          /* help lines; from the 2nd line

> >> > >> onward they

> >> > >> +       char    *help;          /* help lines; from the 2nd line

> >> > >> onward they

> >> > >>                                     are automatically indented */

> >> > >> -        char    *adv_help;      /* advanced help message; from the

> >> > >> 2nd line

> >> > >> +        char    *adv_help;      /* advanced help message; from the

> >> > >> 2nd line

> >> > >

> >> > > Sorry, it's not obvious what changed here. Why is this included?

> >> > >

> >> > >>                                     onward they are

> >> > >> automatically indented */

> >> > >>

> >> > >>         /* the following fields are run-time filled by the

> >> > >> program */ @@ -

> >> > 110,6 +110,11 @@ static struct Command commands[] = {

> >> > >>                 "Send Sanitize command to the <device>.\nThis

> >> > >> will delete the

> >> > unmapped memory region of the device.",

> >> > >>           NULL

> >> > >>         },

> >> > >> +       { do_emmc50_ffu, -2,

> >> > >> +       "emmc50 ffu", "<image path> <device>\n"

> >> > >> +         "run eMMC 5.0 Field firmware update.\n.",

> >> > >

> >> > > Nit: This isn't "run". It's "download firmware to eMMC 5.0

> >> > > compliant

> >> > device".

> >> > >

> >> > >> +         NULL

> >> > >> +       },

> >> > >>         { 0, 0, 0, 0 }

> >> > >>  };

> >> > >>

> >> > >> @@ -362,7 +367,7 @@ static int parse_args(int argc, char **argv,

> >> > >>                         matchcmd->verb, matchcmd->nargs);

> >> > >>                         return -2;

> >> > >>         }

> >> > >> -

> >> > >> +

> >> > >

> >> > > I'm going to ignore white space mangle on this patch and assume

> >> > > you'll ask if you need help using gmail to send patches using git send-

> email.

> >> > >

> >> > > But the above isn't white space mangle caused by email - it's

> >> > > part of this patch and I'm not seeing a difference in this

> >> > > <REDACTED> gmail editor.

> >> > >

> >> > >>          if (prepare_args( nargs_, args_, prgname, matchcmd )){

> >> > >>                  fprintf(stderr, "ERROR: not enough memory\\n");

> >> > >>                 return -20;

> >> > >> diff --git a/mmc.h b/mmc.h

> >> > >> index 9871d62..3be6db0 100644

> >> > >> --- a/mmc.h

> >> > >> +++ b/mmc.h

> >> > >> @@ -80,6 +80,14 @@

> >> > >>  #define BKOPS_ENABLE   (1<<0)

> >> > >>

> >> > >>  /*

> >> > >> + * sector size

> >> > >> +*/

> >> > >> +#define CARD_BLOCK_SIZE        512

> >> > >

> >> > > sector size is advertised by the device. It could be either 512

> >> > > or 4K bytes.

> >> > No?

> >> > >

> >> > > "7.4.17 NUMBER_OF_FW_SECTORS_CORRECTLY_PROGRAMMED

> [305-302]

> >> > >

> >> > > The value is in terms of 512 Bytes or in multiple of eight

> >> > > 512Bytes sectors (4KBytes) depending on the value of the

> >> > > DATA_SECTOR_SIZE field."

> >> > >

> >> > > I don't think this should be hard coded to 512. And a few places

> >> > > I see hard coded with "<< 9" will likely need to take this into account.

> >> > >

> >> > >

> >> > >> +

> >> > >> +#define FFU_DWONLOAD_OP        302

> >> > >> +#define FFU_INSTALL_OP 303

> >> > >

> >> > > These should match kernel definitions (complete name and value).

> >> > >

> >> > >> +

> >> > >> +/*

> >> > >>   * EXT_CSD field definitions

> >> > >>   */

> >> > >>  #define EXT_CSD_HPI_SUPP               (1<<0)

> >> > >> diff --git a/mmc_cmds.c b/mmc_cmds.c index b8afa74..24c4a6b

> >> > >> 100644

> >> > >> --- a/mmc_cmds.c

> >> > >> +++ b/mmc_cmds.c

> >> > >> @@ -1163,3 +1163,112 @@ int do_sanitize(int nargs, char **argv)

> >> > >>

> >> > >>  }

> >> > >>

> >> > >> +static int ffu_download_image(int fw_fd, int mmc_fd) {

> >> > >> +       int ret = 0;

> >> > >> +       struct mmc_ioc_cmd mmc_ioc_cmd;

> >> > >> +       char data_buff[MMC_IOC_MAX_BYTES];

> >> > >> +       int file_size;

> >> > >

> >> > > This should be off_t type. See "man 2 lseek".

> >> > >

> >> > >> +       int size;

> >> > >

> >> > > This should be size_t type.  See "man 2 read".

> >> > >

> >> > >> +       int data_length;

> >> > >

> >> > > This should ssize_t type. See "man 2 read".

> >> > >

> >> > >> +

> >> > >> +       memset(data_buff, 0, sizeof(data_buff));

> >> > >> +       /* get file size */

> >> > >> +       file_size = lseek(fw_fd, 0, SEEK_END);

> >> > >

> >> > > I'm wondering why lseek would be preferred over fstat().

> >> > >

> >> > >> +       if (file_size < 0) {

> >> > >> +               ret =  -1;

> >> > >> +               perror("seek file error \n");

> >> > >> +               goto exit;

> >> > >> +       }

> >> > >> +

> >> > >> +       lseek(fw_fd, 0, SEEK_SET);

> >> > >> +       do {

> >> > >> +               size = (file_size > MMC_IOC_MAX_BYTES) ?

> >> > >> +                               MMC_IOC_MAX_BYTES : file_size;

> >> > >> +               /* Read FW data from file */

> >> > >> +               data_length = read(fw_fd, data_buff, size);

> >> > >> +               if (data_length == -1) {

> >> > >

> >> > > Should this test data_length < size ?

> >> > >

> >> > >> +                       ret = -1;

> >> > >> +                       goto exit;

> >> > >> +               }

> >> > >

> >> > > Gwendal and Kees (CC'd) would prefer to send the file name to the

> >> > > kernel as part of the ioctl and use existing udev mechanisms to

> >> > > request the firmware.

> >> >

> >> > Yes, please see Documentation/firmware_class/README for

> information

> >> > on the kernel internals, but I would much prefer the kernel do all

> >> > the loading, not userspace. The kernel driver can request the

> >> > firmware

> >> > contents:

> >> >

> >> >          if(request_firmware(&fw_entry, $FIRMWARE, device) == 0)

> >> >                 copy_fw_to_device(fw_entry->data, fw_entry->size);

> >> >          release(fw_entry);

> >> >

> >> > and then send it to the device. Doing this from userspace means

> >> > there is no way to verify the firmware contents. Equally, the

> >> > kernel should actively block the MMC_FFU_DOWNLOAD_OP op, since it

> >> > should be considered a sensitive operation.

> >>

> >> Indeed this mechanism allows to download FW file directly to the

> >> driver, and not using IOCTL for this.

> >>

> >> But actually, eMMC5.0 spec does not requires any of FW file content

> >> to be verified by the host.

> >> The FW file should  be downloaded entirely and verified by eMMC

> >> device internally.

> >

> >

> > The point of using firmware_request() is the firmware image is not

> > sent within the IOCTL itself but provided by a well known daemon, in

> > this case udevd.

> > We can make udevd the gate keeper of firmware images [not only eMMC

> > devices, but wifi and 3g modems as well] and ensure that only approved

> > and well-known images are sent to the devices.

> 

> Not only udevd, but the kernel itself -- the kernel too. The goal is to make the

> kernel the trusted element, and to not trust any portion of userspace that

> can't be cryptographically verified.

> 

> -Kees

> 


We will consider using udev mechanism. But first we need to make a short research on this topic.
As well as we need to understand if udev is also used in mobile platforms environment.
Are you familiar with udev usage on mobile platforms?


> >

> > Gwendal.

> >>

> >>

> >> Please let us know if you aware of other solutions, which are

> >> requires FW file verification in the host side.

> >>

> >> In the way that we have implemented FW download routine in the

> >> driver, the FW download process is blocked (using claim_host())

> >> anyway, and prevents interruptions of this process by other IO requests.

> >>

> >> >

> >> > -Kees

> >> >

> >> > >

> >> > > This has some advantages for security which make it a lot harder

> >> > > to "plant" the hacked firmware on devices. I'll let Gwendal and

> >> > > Kees present the details of those ideas.

> >> > >

> >> > >> +               /* prepare and send ioctl */

> >> > >> +               memset(&mmc_ioc_cmd, 0, sizeof(mmc_ioc_cmd));

> >> > >> +               mmc_ioc_cmd.opcode =  FFU_DWONLOAD_OP;

> >> > >> +               mmc_ioc_cmd.blksz = CARD_BLOCK_SIZE;

> >> > >> +               mmc_ioc_cmd.blocks = data_length / mmc_ioc_cmd.blksz;

> >> > >> +               mmc_ioc_cmd.arg =  0;

> >> > >> +               mmc_ioc_cmd.flags = MMC_RSP_SPI_R1 | MMC_RSP_R1

> >> > >> + |

> >> > MMC_CMD_ADTC;

> >> > >> +               mmc_ioc_cmd.write_flag = 1;

> >> > >> +               mmc_ioc_cmd_set_data(mmc_ioc_cmd, data_buff);

> >> > >> +               ret = ioctl(mmc_fd, MMC_IOC_CMD, &mmc_ioc_cmd);

> >> > >> +               if (ret) {

> >> > >> +                       perror("ioctl FW download");

> >> > >> +                       goto exit;

> >> > >> +               }

> >> > >> +

> >> > >> +               file_size = file_size - size;

> >> > >> +               printf("firmware file loading, remaining:   %d\n",

> >> > >> file_size);

> >> > >> +       } while (file_size > 0);

> >> > >> +

> >> > >> +exit:

> >> > >> +

> >> > >> +       return ret;

> >> > >> +}

> >> > >> +

> >> > >> +static int ffu_install(int mmc_fd) {

> >> > >> +       int ret;

> >> > >> +       struct mmc_ioc_cmd mmc_ioc_cmd;

> >> > >> +

> >> > >> +       memset(&mmc_ioc_cmd, 0, sizeof(mmc_ioc_cmd));

> >> > >> +       mmc_ioc_cmd.opcode = FFU_INSTALL_OP;

> >> > >> +       mmc_ioc_cmd.blksz = CARD_BLOCK_SIZE;

> >> > >> +       mmc_ioc_cmd.blocks = 0;

> >> > >> +       mmc_ioc_cmd.arg =  0;

> >> > >> +       mmc_ioc_cmd.flags = MMC_RSP_SPI_R1 | MMC_RSP_R1 |

> >> > MMC_CMD_ADTC;

> >> > >> +       mmc_ioc_cmd.write_flag = 0;

> >> > >> +       ret = ioctl(mmc_fd, MMC_IOC_CMD, &mmc_ioc_cmd);

> >> > >> +       if (ret)

> >> > >> +               perror("ioctl install");

> >> > >> +

> >> > >> +       printf("ffu_install ret %d \n", ret);

> >> > >> +       return ret;

> >> > >> +}

> >> > >> +

> >> > >> +int do_emmc50_ffu(int nargs, char **argv) {

> >> > >> +       int fd, fw_fd, ret;

> >> > >> +       char *device;

> >> > >> +

> >> > >> +       CHECK(nargs != 3, "Usage: ffu <image path>

> >> > >> + </path/to/mmcblkX>

> >> > \n",

> >> > >> +                               exit(1));

> >> > >> +

> >> > >> +       device = argv[2];

> >> > >> +       fd = open(device, O_RDWR);

> >> > >> +       if (fd < 0) {

> >> > >> +               perror("open");

> >> > >> +               exit(1);

> >> > >> +       }

> >> > >> +

> >> > >> +       /* open eMMC5.0 firmware image file */

> >> > >> +       fw_fd = open(argv[1], O_RDONLY);

> >> > >> +       if (fw_fd < 0) {

> >> > >> +               perror("open eMMC5.0 firmware file");

> >> > >> +               ret = -1;

> >> > >

> >> > > Don't want to return the errno value?

> >> > >

> >> > >> +               goto exit;

> >> > >> +       }

> >> > >> +

> >> > >> +       ret = ffu_download_image(fw_fd, fd);

> >> > >> +       if (ret)

> >> > >> +               goto exit;

> >> > >> +

> >> > >> +       ret = ffu_install(fd);

> >> > >> +       if (ret)

> >> > >> +               goto exit;

> >> > >> +

> >> > >> +exit:

> >> > >> +       close(fd);

> >> > >> +       close(fw_fd);

> >> > >> +

> >> > >> +       return ret;

> >> > >> +}

> >> > >> diff --git a/mmc_cmds.h b/mmc_cmds.h index f06cc10..77a6cb8

> >> > >> 100644

> >> > >> --- a/mmc_cmds.h

> >> > >> +++ b/mmc_cmds.h

> >> > >> @@ -28,3 +28,5 @@ int do_sanitize(int nargs, char **argv);  int

> >> > >> do_status_get(int nargs, char **argv);  int do_enh_area_set(int

> >> > >> nargs, char **argv);  int do_write_reliability_set(int nargs,

> >> > >> char **argv);

> >> > >> +int do_emmc50_ffu(int nargs, char **argv);

> >> > >> +

> >> > >> --

> >> > >> 1.7.5.4

> >> > >>

> >> > >>

> >> > >> Avi Shchislowski | Staff Software Engineer, MCS Embedded  |

> >> > >> SanDisk |

> >> > >> +972.09.763-2666| www.sandisk.com

> >> > >>

> >> > >

> >> > > cheers,

> >> > > grant

> >> >

> >> >

> >> >

> >> > --

> >> > Kees Cook

> >> > Chrome OS Security

> >>

> >>

> >>

> >

> 

> 

> 

> --

> Kees Cook

> Chrome OS Security

Kees Cook March 10, 2014, 5:25 p.m. UTC | #8

On Mon, Mar 10, 2014 at 1:55 AM, Alex Lemberg <[e-mail]> wrote:
> Hi Kees and Gwendal,
>
> Please see inline.
>
> Thanks,
> Alex
>
>> -----Original Message-----
>> From: [e-mail] [mailto:[e-mail]] On Behalf Of
>> Kees Cook
>> Sent: Wednesday, March 05, 2014 11:59 PM
>> To: Gwendal Grignou
>> Cc: Alex Lemberg; Grant Grundler; Avi Shchislowski; [e-mail]; linux-
>> [e-mail]; Puthikorn Voravootivat
>> Subject: Re: [RFC PATCH 1/1] mmc-utils: Support-sending-eMMC-5.0-FFU
>>
>> On Wed, Mar 5, 2014 at 9:52 AM, Gwendal Grignou
>> <[e-mail]> wrote:
>> >
>> >
>> >
>> > On Wed, Mar 5, 2014 at 8:35 AM, Alex Lemberg
>> > <[e-mail]>
>> > wrote:
>> >>
>> >> Hi Kees,
>> >>
>> >> Thanks for your comment.
>> >> Please see our response inline.
>> >>
>> >>
>> >> > -----Original Message-----
>> >> > From: [e-mail] [mailto:[e-mail]] On
>> Behalf Of
>> >> > Kees Cook
>> >> > Sent: Saturday, March 01, 2014 1:21 AM
>> >> > To: Grant Grundler
>> >> > Cc: Avi Shchislowski; [e-mail]; [e-mail];
>> >> > Alex Lemberg; Gwendal Grignou; Puthikorn Voravootivat
>> >> > Subject: Re: [RFC PATCH 1/1] mmc-utils:
>> >> > Support-sending-eMMC-5.0-FFU
>> >> >
>> >> > On Fri, Feb 28, 2014 at 2:39 PM, Grant Grundler
>> >> > <[e-mail]>
>> >> > wrote:
>> >> > > Avi,
>> >> > > Thanks for posting these - I look forward to seeing this
>> >> > > functionality available in mmc-utils (and kernel as needed).
>> >> > >
>> >> > > Comments as usual inline.
>> >> > >
>> >> > > I've added Gwendal/Kees to CC to comment on security issues of
>> >> > > this proposal. See notes below.
>> >> > >
>> >> > >
>> >> > > On Sun, Feb 9, 2014 at 1:08 AM, Avi Shchislowski
>> >> > > <[e-mail]> wrote:
>> >> > >>   The mmc-utils was modified to invoke eMMC5.0 Field Firmware
>> >> > >> Update
>> >> > (FFU) process in mmc driver
>> >> > >>   New command was add: "do_emmc50_ffu".
>> >> > >>
>> >> > >>   This patch depends on patch  mmc: Support-FFU-for-eMMC-v5.0
>> >> > >> Committed by Avi Shchislowski <[e-mail]>
>> >> > >>
>> >> > >>   FFU will be done in two steps. Two new IOCTL codes will be
>> >> > >> sent to the
>> >> > driver in order to operate FFU code:
>> >> > >>     1.  FFU_DWONLOAD_OP (sent in ffu_download_image() function)
>> >> > >
>> >> > > Any reason for the typo? DOWNLOAD maybe?
>> >> > > Shouldn't that be MMC_FFU_DOWNLOAD_OP to match the proposed
>> >> > kernel definition?
>> >> > >
>> >> > >>         2.  FFU_INSTALL_OP (sent in ffu_install() function)
>> >> > >
>> >> > > Ditto: MMC_FFU_INSTALL_OP
>> >> > >
>> >> > >>
>> >> > >>
>> >> > >> Signed-off-by: Avi Shchislowski <[e-mail]>
>> >> > >> Signed-off-by: Alex Lemberg <[e-mail]>
>> >> > >>
>> >> > >> diff --git a/mmc.c b/mmc.c
>> >> > >> index 926e92f..a01852d 100644
>> >> > >> --- a/mmc.c
>> >> > >> +++ b/mmc.c
>> >> > >> @@ -36,9 +36,9 @@ struct Command {
>> >> > >>                                    if >= 0, number of arguments,
>> >> > >>                                    if < 0, _minimum_ number of
>> >> > >> arguments */
>> >> > >>         char    *verb;          /* verb */
>> >> > >> -       char    *help;          /* help lines; from the 2nd line
>> >> > >> onward they
>> >> > >> +       char    *help;          /* help lines; from the 2nd line
>> >> > >> onward they
>> >> > >>                                     are automatically indented */
>> >> > >> -        char    *adv_help;      /* advanced help message; from the
>> >> > >> 2nd line
>> >> > >> +        char    *adv_help;      /* advanced help message; from the
>> >> > >> 2nd line
>> >> > >
>> >> > > Sorry, it's not obvious what changed here. Why is this included?
>> >> > >
>> >> > >>                                     onward they are
>> >> > >> automatically indented */
>> >> > >>
>> >> > >>         /* the following fields are run-time filled by the
>> >> > >> program */ @@ -
>> >> > 110,6 +110,11 @@ static struct Command commands[] = {
>> >> > >>                 "Send Sanitize command to the <device>.\nThis
>> >> > >> will delete the
>> >> > unmapped memory region of the device.",
>> >> > >>           NULL
>> >> > >>         },
>> >> > >> +       { do_emmc50_ffu, -2,
>> >> > >> +       "emmc50 ffu", "<image path> <device>\n"
>> >> > >> +         "run eMMC 5.0 Field firmware update.\n.",
>> >> > >
>> >> > > Nit: This isn't "run". It's "download firmware to eMMC 5.0
>> >> > > compliant
>> >> > device".
>> >> > >
>> >> > >> +         NULL
>> >> > >> +       },
>> >> > >>         { 0, 0, 0, 0 }
>> >> > >>  };
>> >> > >>
>> >> > >> @@ -362,7 +367,7 @@ static int parse_args(int argc, char **argv,
>> >> > >>                         matchcmd->verb, matchcmd->nargs);
>> >> > >>                         return -2;
>> >> > >>         }
>> >> > >> -
>> >> > >> +
>> >> > >
>> >> > > I'm going to ignore white space mangle on this patch and assume
>> >> > > you'll ask if you need help using gmail to send patches using git send-
>> email.
>> >> > >
>> >> > > But the above isn't white space mangle caused by email - it's
>> >> > > part of this patch and I'm not seeing a difference in this
>> >> > > <REDACTED> gmail editor.
>> >> > >
>> >> > >>          if (prepare_args( nargs_, args_, prgname, matchcmd )){
>> >> > >>                  fprintf(stderr, "ERROR: not enough memory\\n");
>> >> > >>                 return -20;
>> >> > >> diff --git a/mmc.h b/mmc.h
>> >> > >> index 9871d62..3be6db0 100644
>> >> > >> --- a/mmc.h
>> >> > >> +++ b/mmc.h
>> >> > >> @@ -80,6 +80,14 @@
>> >> > >>  #define BKOPS_ENABLE   (1<<0)
>> >> > >>
>> >> > >>  /*
>> >> > >> + * sector size
>> >> > >> +*/
>> >> > >> +#define CARD_BLOCK_SIZE        512
>> >> > >
>> >> > > sector size is advertised by the device. It could be either 512
>> >> > > or 4K bytes.
>> >> > No?
>> >> > >
>> >> > > "7.4.17 NUMBER_OF_FW_SECTORS_CORRECTLY_PROGRAMMED
>> [305-302]
>> >> > >
>> >> > > The value is in terms of 512 Bytes or in multiple of eight
>> >> > > 512Bytes sectors (4KBytes) depending on the value of the
>> >> > > DATA_SECTOR_SIZE field."
>> >> > >
>> >> > > I don't think this should be hard coded to 512. And a few places
>> >> > > I see hard coded with "<< 9" will likely need to take this into account.
>> >> > >
>> >> > >
>> >> > >> +
>> >> > >> +#define FFU_DWONLOAD_OP        302
>> >> > >> +#define FFU_INSTALL_OP 303
>> >> > >
>> >> > > These should match kernel definitions (complete name and value).
>> >> > >
>> >> > >> +
>> >> > >> +/*
>> >> > >>   * EXT_CSD field definitions
>> >> > >>   */
>> >> > >>  #define EXT_CSD_HPI_SUPP               (1<<0)
>> >> > >> diff --git a/mmc_cmds.c b/mmc_cmds.c index b8afa74..24c4a6b
>> >> > >> 100644
>> >> > >> --- a/mmc_cmds.c
>> >> > >> +++ b/mmc_cmds.c
>> >> > >> @@ -1163,3 +1163,112 @@ int do_sanitize(int nargs, char **argv)
>> >> > >>
>> >> > >>  }
>> >> > >>
>> >> > >> +static int ffu_download_image(int fw_fd, int mmc_fd) {
>> >> > >> +       int ret = 0;
>> >> > >> +       struct mmc_ioc_cmd mmc_ioc_cmd;
>> >> > >> +       char data_buff[MMC_IOC_MAX_BYTES];
>> >> > >> +       int file_size;
>> >> > >
>> >> > > This should be off_t type. See "man 2 lseek".
>> >> > >
>> >> > >> +       int size;
>> >> > >
>> >> > > This should be size_t type.  See "man 2 read".
>> >> > >
>> >> > >> +       int data_length;
>> >> > >
>> >> > > This should ssize_t type. See "man 2 read".
>> >> > >
>> >> > >> +
>> >> > >> +       memset(data_buff, 0, sizeof(data_buff));
>> >> > >> +       /* get file size */
>> >> > >> +       file_size = lseek(fw_fd, 0, SEEK_END);
>> >> > >
>> >> > > I'm wondering why lseek would be preferred over fstat().
>> >> > >
>> >> > >> +       if (file_size < 0) {
>> >> > >> +               ret =  -1;
>> >> > >> +               perror("seek file error \n");
>> >> > >> +               goto exit;
>> >> > >> +       }
>> >> > >> +
>> >> > >> +       lseek(fw_fd, 0, SEEK_SET);
>> >> > >> +       do {
>> >> > >> +               size = (file_size > MMC_IOC_MAX_BYTES) ?
>> >> > >> +                               MMC_IOC_MAX_BYTES : file_size;
>> >> > >> +               /* Read FW data from file */
>> >> > >> +               data_length = read(fw_fd, data_buff, size);
>> >> > >> +               if (data_length == -1) {
>> >> > >
>> >> > > Should this test data_length < size ?
>> >> > >
>> >> > >> +                       ret = -1;
>> >> > >> +                       goto exit;
>> >> > >> +               }
>> >> > >
>> >> > > Gwendal and Kees (CC'd) would prefer to send the file name to the
>> >> > > kernel as part of the ioctl and use existing udev mechanisms to
>> >> > > request the firmware.
>> >> >
>> >> > Yes, please see Documentation/firmware_class/README for
>> information
>> >> > on the kernel internals, but I would much prefer the kernel do all
>> >> > the loading, not userspace. The kernel driver can request the
>> >> > firmware
>> >> > contents:
>> >> >
>> >> >          if(request_firmware(&fw_entry, $FIRMWARE, device) == 0)
>> >> >                 copy_fw_to_device(fw_entry->data, fw_entry->size);
>> >> >          release(fw_entry);
>> >> >
>> >> > and then send it to the device. Doing this from userspace means
>> >> > there is no way to verify the firmware contents. Equally, the
>> >> > kernel should actively block the MMC_FFU_DOWNLOAD_OP op, since it
>> >> > should be considered a sensitive operation.
>> >>
>> >> Indeed this mechanism allows to download FW file directly to the
>> >> driver, and not using IOCTL for this.
>> >>
>> >> But actually, eMMC5.0 spec does not requires any of FW file content
>> >> to be verified by the host.
>> >> The FW file should  be downloaded entirely and verified by eMMC
>> >> device internally.
>> >
>> >
>> > The point of using firmware_request() is the firmware image is not
>> > sent within the IOCTL itself but provided by a well known daemon, in
>> > this case udevd.
>> > We can make udevd the gate keeper of firmware images [not only eMMC
>> > devices, but wifi and 3g modems as well] and ensure that only approved
>> > and well-known images are sent to the devices.
>>
>> Not only udevd, but the kernel itself -- the kernel too. The goal is to make the
>> kernel the trusted element, and to not trust any portion of userspace that
>> can't be cryptographically verified.
>>
>> -Kees
>>
>
> We will consider using udev mechanism. But first we need to make a short research on this topic.
> As well as we need to understand if udev is also used in mobile platforms environment.
> Are you familiar with udev usage on mobile platforms?

I think it's very likely that there is a kernel event notification
handler (usually udev) running on all Linux devices. That would be the
software responsible for servicing the firmware request.

-Kees

>
>
>> >
>> > Gwendal.
>> >>
>> >>
>> >> Please let us know if you aware of other solutions, which are
>> >> requires FW file verification in the host side.
>> >>
>> >> In the way that we have implemented FW download routine in the
>> >> driver, the FW download process is blocked (using claim_host())
>> >> anyway, and prevents interruptions of this process by other IO requests.
>> >>
>> >> >
>> >> > -Kees
>> >> >
>> >> > >
>> >> > > This has some advantages for security which make it a lot harder
>> >> > > to "plant" the hacked firmware on devices. I'll let Gwendal and
>> >> > > Kees present the details of those ideas.
>> >> > >
>> >> > >> +               /* prepare and send ioctl */
>> >> > >> +               memset(&mmc_ioc_cmd, 0, sizeof(mmc_ioc_cmd));
>> >> > >> +               mmc_ioc_cmd.opcode =  FFU_DWONLOAD_OP;
>> >> > >> +               mmc_ioc_cmd.blksz = CARD_BLOCK_SIZE;
>> >> > >> +               mmc_ioc_cmd.blocks = data_length / mmc_ioc_cmd.blksz;
>> >> > >> +               mmc_ioc_cmd.arg =  0;
>> >> > >> +               mmc_ioc_cmd.flags = MMC_RSP_SPI_R1 | MMC_RSP_R1
>> >> > >> + |
>> >> > MMC_CMD_ADTC;
>> >> > >> +               mmc_ioc_cmd.write_flag = 1;
>> >> > >> +               mmc_ioc_cmd_set_data(mmc_ioc_cmd, data_buff);
>> >> > >> +               ret = ioctl(mmc_fd, MMC_IOC_CMD, &mmc_ioc_cmd);
>> >> > >> +               if (ret) {
>> >> > >> +                       perror("ioctl FW download");
>> >> > >> +                       goto exit;
>> >> > >> +               }
>> >> > >> +
>> >> > >> +               file_size = file_size - size;
>> >> > >> +               printf("firmware file loading, remaining:   %d\n",
>> >> > >> file_size);
>> >> > >> +       } while (file_size > 0);
>> >> > >> +
>> >> > >> +exit:
>> >> > >> +
>> >> > >> +       return ret;
>> >> > >> +}
>> >> > >> +
>> >> > >> +static int ffu_install(int mmc_fd) {
>> >> > >> +       int ret;
>> >> > >> +       struct mmc_ioc_cmd mmc_ioc_cmd;
>> >> > >> +
>> >> > >> +       memset(&mmc_ioc_cmd, 0, sizeof(mmc_ioc_cmd));
>> >> > >> +       mmc_ioc_cmd.opcode = FFU_INSTALL_OP;
>> >> > >> +       mmc_ioc_cmd.blksz = CARD_BLOCK_SIZE;
>> >> > >> +       mmc_ioc_cmd.blocks = 0;
>> >> > >> +       mmc_ioc_cmd.arg =  0;
>> >> > >> +       mmc_ioc_cmd.flags = MMC_RSP_SPI_R1 | MMC_RSP_R1 |
>> >> > MMC_CMD_ADTC;
>> >> > >> +       mmc_ioc_cmd.write_flag = 0;
>> >> > >> +       ret = ioctl(mmc_fd, MMC_IOC_CMD, &mmc_ioc_cmd);
>> >> > >> +       if (ret)
>> >> > >> +               perror("ioctl install");
>> >> > >> +
>> >> > >> +       printf("ffu_install ret %d \n", ret);
>> >> > >> +       return ret;
>> >> > >> +}
>> >> > >> +
>> >> > >> +int do_emmc50_ffu(int nargs, char **argv) {
>> >> > >> +       int fd, fw_fd, ret;
>> >> > >> +       char *device;
>> >> > >> +
>> >> > >> +       CHECK(nargs != 3, "Usage: ffu <image path>
>> >> > >> + </path/to/mmcblkX>
>> >> > \n",
>> >> > >> +                               exit(1));
>> >> > >> +
>> >> > >> +       device = argv[2];
>> >> > >> +       fd = open(device, O_RDWR);
>> >> > >> +       if (fd < 0) {
>> >> > >> +               perror("open");
>> >> > >> +               exit(1);
>> >> > >> +       }
>> >> > >> +
>> >> > >> +       /* open eMMC5.0 firmware image file */
>> >> > >> +       fw_fd = open(argv[1], O_RDONLY);
>> >> > >> +       if (fw_fd < 0) {
>> >> > >> +               perror("open eMMC5.0 firmware file");
>> >> > >> +               ret = -1;
>> >> > >
>> >> > > Don't want to return the errno value?
>> >> > >
>> >> > >> +               goto exit;
>> >> > >> +       }
>> >> > >> +
>> >> > >> +       ret = ffu_download_image(fw_fd, fd);
>> >> > >> +       if (ret)
>> >> > >> +               goto exit;
>> >> > >> +
>> >> > >> +       ret = ffu_install(fd);
>> >> > >> +       if (ret)
>> >> > >> +               goto exit;
>> >> > >> +
>> >> > >> +exit:
>> >> > >> +       close(fd);
>> >> > >> +       close(fw_fd);
>> >> > >> +
>> >> > >> +       return ret;
>> >> > >> +}
>> >> > >> diff --git a/mmc_cmds.h b/mmc_cmds.h index f06cc10..77a6cb8
>> >> > >> 100644
>> >> > >> --- a/mmc_cmds.h
>> >> > >> +++ b/mmc_cmds.h
>> >> > >> @@ -28,3 +28,5 @@ int do_sanitize(int nargs, char **argv);  int
>> >> > >> do_status_get(int nargs, char **argv);  int do_enh_area_set(int
>> >> > >> nargs, char **argv);  int do_write_reliability_set(int nargs,
>> >> > >> char **argv);
>> >> > >> +int do_emmc50_ffu(int nargs, char **argv);
>> >> > >> +
>> >> > >> --
>> >> > >> 1.7.5.4
>> >> > >>
>> >> > >>
>> >> > >> Avi Shchislowski | Staff Software Engineer, MCS Embedded  |
>> >> > >> SanDisk |
>> >> > >> +972.09.763-2666| www.sandisk.com
>> >> > >>
>> >> > >
>> >> > > cheers,
>> >> > > grant
>> >> >
>> >> >
>> >> >
>> >> > --
>> >> > Kees Cook
>> >> > Chrome OS Security
>> >>
>> >>
>> >>
>> >
>>
>>
>>
>> --
>> Kees Cook
>> Chrome OS Security
>
>
>

Patch

diff --git a/mmc.c b/mmc.c
index 926e92f..a01852d 100644
--- a/mmc.c
+++ b/mmc.c
@@ -36,9 +36,9 @@  struct Command {
                                   if >= 0, number of arguments,
                                   if < 0, _minimum_ number of arguments */
        char    *verb;          /* verb */
-       char    *help;          /* help lines; from the 2nd line onward they
+       char    *help;          /* help lines; from the 2nd line onward they
                                    are automatically indented */
-        char    *adv_help;      /* advanced help message; from the 2nd line
+        char    *adv_help;      /* advanced help message; from the 2nd line
                                    onward they are automatically indented */

        /* the following fields are run-time filled by the program */ @@ -110,6 +110,11 @@ static struct Command commands[] = {
                "Send Sanitize command to the <device>.\nThis will delete the unmapped memory region of the device.",
          NULL
        },
+       { do_emmc50_ffu, -2,
+       "emmc50 ffu", "<image path> <device>\n"
+         "run eMMC 5.0 Field firmware update.\n.",
+         NULL
+       },
        { 0, 0, 0, 0 }
 };

@@ -362,7 +367,7 @@  static int parse_args(int argc, char **argv,
                        matchcmd->verb, matchcmd->nargs);
                        return -2;
        }
-
+
         if (prepare_args( nargs_, args_, prgname, matchcmd )){
                 fprintf(stderr, "ERROR: not enough memory\\n");
                return -20;
diff --git a/mmc.h b/mmc.h
index 9871d62..3be6db0 100644
--- a/mmc.h
+++ b/mmc.h
@@ -80,6 +80,14 @@ 
 #define BKOPS_ENABLE   (1<<0)

 /*
+ * sector size
+*/
+#define CARD_BLOCK_SIZE        512
+
+#define FFU_DWONLOAD_OP        302
+#define FFU_INSTALL_OP 303
+
+/*
  * EXT_CSD field definitions
  */
 #define EXT_CSD_HPI_SUPP               (1<<0)
diff --git a/mmc_cmds.c b/mmc_cmds.c
index b8afa74..24c4a6b 100644
--- a/mmc_cmds.c
+++ b/mmc_cmds.c
@@ -1163,3 +1163,112 @@  int do_sanitize(int nargs, char **argv)

 }

+static int ffu_download_image(int fw_fd, int mmc_fd) {
+       int ret = 0;
+       struct mmc_ioc_cmd mmc_ioc_cmd;
+       char data_buff[MMC_IOC_MAX_BYTES];
+       int file_size;
+       int size;
+       int data_length;
+
+       memset(data_buff, 0, sizeof(data_buff));
+       /* get file size */
+       file_size = lseek(fw_fd, 0, SEEK_END);
+       if (file_size < 0) {
+               ret =  -1;
+               perror("seek file error \n");
+               goto exit;
+       }
+
+       lseek(fw_fd, 0, SEEK_SET);
+       do {
+               size = (file_size > MMC_IOC_MAX_BYTES) ?
+                               MMC_IOC_MAX_BYTES : file_size;
+               /* Read FW data from file */
+               data_length = read(fw_fd, data_buff, size);
+               if (data_length == -1) {
+                       ret = -1;
+                       goto exit;
+               }
+               /* prepare and send ioctl */
+               memset(&mmc_ioc_cmd, 0, sizeof(mmc_ioc_cmd));
+               mmc_ioc_cmd.opcode =  FFU_DWONLOAD_OP;
+               mmc_ioc_cmd.blksz = CARD_BLOCK_SIZE;
+               mmc_ioc_cmd.blocks = data_length / mmc_ioc_cmd.blksz;
+               mmc_ioc_cmd.arg =  0;
+               mmc_ioc_cmd.flags = MMC_RSP_SPI_R1 | MMC_RSP_R1 | MMC_CMD_ADTC;
+               mmc_ioc_cmd.write_flag = 1;
+               mmc_ioc_cmd_set_data(mmc_ioc_cmd, data_buff);
+               ret = ioctl(mmc_fd, MMC_IOC_CMD, &mmc_ioc_cmd);
+               if (ret) {
+                       perror("ioctl FW download");
+                       goto exit;
+               }
+
+               file_size = file_size - size;
+               printf("firmware file loading, remaining:   %d\n", file_size);
+       } while (file_size > 0);
+
+exit:
+
+       return ret;
+}
+
+static int ffu_install(int mmc_fd)
+{
+       int ret;
+       struct mmc_ioc_cmd mmc_ioc_cmd;
+
+       memset(&mmc_ioc_cmd, 0, sizeof(mmc_ioc_cmd));
+       mmc_ioc_cmd.opcode = FFU_INSTALL_OP;
+       mmc_ioc_cmd.blksz = CARD_BLOCK_SIZE;
+       mmc_ioc_cmd.blocks = 0;
+       mmc_ioc_cmd.arg =  0;
+       mmc_ioc_cmd.flags = MMC_RSP_SPI_R1 | MMC_RSP_R1 | MMC_CMD_ADTC;
+       mmc_ioc_cmd.write_flag = 0;
+       ret = ioctl(mmc_fd, MMC_IOC_CMD, &mmc_ioc_cmd);
+       if (ret)
+               perror("ioctl install");
+
+       printf("ffu_install ret %d \n", ret);
+       return ret;
+}
+
+int do_emmc50_ffu(int nargs, char **argv) {
+       int fd, fw_fd, ret;
+       char *device;
+
+       CHECK(nargs != 3, "Usage: ffu <image path> </path/to/mmcblkX> \n",
+                               exit(1));
+
+       device = argv[2];
+       fd = open(device, O_RDWR);
+       if (fd < 0) {
+               perror("open");
+               exit(1);
+       }
+
+       /* open eMMC5.0 firmware image file */
+       fw_fd = open(argv[1], O_RDONLY);
+       if (fw_fd < 0) {
+               perror("open eMMC5.0 firmware file");
+               ret = -1;
+               goto exit;
+       }
+
+       ret = ffu_download_image(fw_fd, fd);
+       if (ret)
+               goto exit;
+
+       ret = ffu_install(fd);
+       if (ret)
+               goto exit;
+
+exit:
+       close(fd);
+       close(fw_fd);
+
+       return ret;
+}
diff --git a/mmc_cmds.h b/mmc_cmds.h
index f06cc10..77a6cb8 100644
--- a/mmc_cmds.h
+++ b/mmc_cmds.h
@@ -28,3 +28,5 @@  int do_sanitize(int nargs, char **argv);  int do_status_get(int nargs, char **argv);  int do_enh_area_set(int nargs, char **argv);  int do_write_reliability_set(int nargs, char **argv);
+int do_emmc50_ffu(int nargs, char **argv);
+
--
1.7.5.4