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 > > 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 -- 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
