Hi Grant, Thanks for your review and comments. Please see our response inline. > -----Original Message----- > From: grundler@xxxxxxxxxx [mailto:grundler@xxxxxxxxxx] On Behalf Of > Grant Grundler > Sent: Saturday, March 01, 2014 12:40 AM > To: Avi Shchislowski > Cc: cjb@xxxxxxxxxx; linux-mmc@xxxxxxxxxxxxxxx; Alex Lemberg; Grant > Grundler; Gwendal Grignou; Puthikorn Voravootivat; Kees Cook > Subject: Re: [RFC PATCH 1/1] mmc-utils: Support-sending-eMMC-5.0-FFU > > 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) > Typo > Any reason for the typo? DOWNLOAD maybe? > Shouldn't that be MMC_FFU_DOWNLOAD_OP to match the proposed kernel > definition? Agree. Changing to MMC_FFU_DOWNLOAD_OP. > > > 2. FFU_INSTALL_OP (sent in ffu_install() function) > > Ditto: MMC_FFU_INSTALL_OP > Agree. Changing to 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 */ We will remove this. > > > > /* 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". OK > > > + 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. Thanks, we are checking this... > > > 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. Agree. We will set this value from the EXT_CSD_DATA_SECTOR_SIZE (Please note - this requires additional operation of reading EXT_CSD). > > > > + > > +#define FFU_DWONLOAD_OP 302 > > +#define FFU_INSTALL_OP 303 > > These should match kernel definitions (complete name and value). Agree > > > + > > +/* > > * 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". Right > > > + int size; > > This should be size_t type. See "man 2 read". Right > > > + int data_length; > > This should ssize_t type. See "man 2 read". Right > > > + > > + 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(). No special reason. Changing this to use 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 ? In case we can't rely on status returned by read(), this condition (data_length < size) is better. > > > + 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. OK, We will refer to Gwendal and Kees comments. > > > + /* 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? OK. Will do. > > > + 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 ��.n��������+%������w��{.n�����{��i��)�� jg��� �����ݢj����G���� ���j:+v���w�m������w�������h�����٥
