On Fri, Feb 28, 2014 at 2:39 PM, Grant Grundler <grundler@xxxxxxxxxxxx> 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
> <Avi.Shchislowski@xxxxxxxxxxx> 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 <avi.shchislowski@xxxxxxxxxxx>
>>
>> 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 <avi.shchislowski@xxxxxxxxxxx>
>> Signed-off-by: Alex Lemberg <alex.lemberg@xxxxxxxxxxx>
>>
>> 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
--
Kees Cook
Chrome OS Security
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
