Hi Kees and Gwendal, Please see inline. Thanks, Alex > -----Original Message----- > From: keescook@xxxxxxxxxx [mailto:keescook@xxxxxxxxxx] On Behalf Of > Kees Cook > Sent: Wednesday, March 05, 2014 11:59 PM > To: Gwendal Grignou > Cc: Alex Lemberg; Grant Grundler; Avi Shchislowski; cjb@xxxxxxxxxx; linux- > mmc@xxxxxxxxxxxxxxx; 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 > <gwendal@xxxxxxxxxxxx> wrote: > > > > > > > > On Wed, Mar 5, 2014 at 8:35 AM, Alex Lemberg > > <Alex.Lemberg@xxxxxxxxxxx> > > wrote: > >> > >> Hi Kees, > >> > >> Thanks for your comment. > >> Please see our response inline. > >> > >> > >> > -----Original Message----- > >> > From: keescook@xxxxxxxxxx [mailto:keescook@xxxxxxxxxx] On > Behalf Of > >> > Kees Cook > >> > Sent: Saturday, March 01, 2014 1:21 AM > >> > To: Grant Grundler > >> > Cc: Avi Shchislowski; cjb@xxxxxxxxxx; linux-mmc@xxxxxxxxxxxxxxx; > >> > 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 > >> > <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. > >> > >> 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 ��.n��������+%������w��{.n�����{��i��)�� jg��� �����ݢj����G���� ���j:+v���w�m������w�������h�����٥
