| Message ID | 7a2fd4e7-84b5-8e44-3789-e9ddffe30f64@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | mmc-utils: RPMB fails with status 0x0001 on system eMMC chips | expand |
+ Chris On 27 August 2018 at 11:27, Nikita Maslov <wkernelteam@gmail.com> wrote: > There is a problem with RPMB access on systems which use eMMC as > default storage. > > According to specs, RPMB requires multiple commands to be sent > right one after another, in another case, if there are some other > commands between them, RPMB will fail with status 0x0001. This case > is common for systems which use eMMC as default system storage. > > In current version of mmc-utils RPMB operations are performed as > a sequence of MMC_IOC_CMD ioctl calls. Between these calls eMMC is > unlocked and may be used by mmc driver, so RPMB session will be broken. > > In Linux 4.4+, there is MMC_IOC_MULTI_CMD ioctl which allows to perform > multiple IOC_CMDs in one request. This is a better way to work with > RPMB, because eMMC is supposed to be locked during ioctl. > > I included the patch which tries to use MMC_IOC_MULTI_CMD for RPMB > operations. If it fails with EINVAL, it tries to use MMC_IOC_CMD > (for compatibility). > > This patch was tested on ARM systems (armel) with eMMC chips from > Micron and SanDisk. > > All suggestions are welcomed. > > P.S. This patch was sent to Debian bug tracking system but was left > without reply, so I decided to send it here. > > -- System Information: > Debian Release: buster/sid > APT prefers testing > APT policy: (900, 'testing'), (500, 'testing-debug'), (10, 'unstable') > Architecture: amd64 (x86_64) > Foreign Architectures: i386 > > Kernel: Linux 4.14.0-3-amd64 (SMP w/4 CPU cores) > Locale: LANG=en_US.UTF-8, LC_CTYPE=en_US.UTF-8 (charmap=UTF-8), > LANGUAGE=en_US:en (charmap=UTF-8) > Shell: /bin/sh linked to /bin/dash > Init: systemd (via /run/systemd/system) > LSM: AppArmor: enabled > > Versions of packages mmc-utils depends on: > ii libc6 2.26-6 > > mmc-utils recommends no packages. > > mmc-utils suggests no packages. > > -- no debconf information > > Author: Nikita Maslov <wkernelt...@gmail.com> > Description: Use MMC_IOC_MULTI_CMD for RPMB access > On some systems which use MMC as a main storage device > it is possible that RPMB commands are mixed with > generic MMC access commands which invalidates RPMB. > This patch uses MMC_IOC_MULTI_CMD. > . > mmc-utils (0+git20170901.37c86e60-1) unstable; urgency=medium > . > * Snapshot, taken from the master (20170901). > * Update supporting version of debhelper to 10. > * Update Standards-Version to 4.1.0. > > Last-Update: 2018-03-07 > > --- > > --- mmc-utils-0+git20170901.37c86e60.orig/mmc_cmds.c > +++ mmc-utils-0+git20170901.37c86e60/mmc_cmds.c > @@ -1799,6 +1799,8 @@ int do_sanitize(int nargs, char **argv) > ret; > \ > }) > +#define RMPB_MULTI_CMD_MAX_CMDS 3 > + > enum rpmb_op_type { > MMC_RPMB_WRITE_KEY = 0x01, > MMC_RPMB_READ_CNT = 0x02, > @@ -1837,8 +1839,9 @@ static int do_rpmb_op(int fd, > { > int err; > u_int16_t rpmb_type; > + unsigned i; > - struct mmc_ioc_cmd ioc = { > + struct mmc_ioc_cmd ioc_base = { > .arg = 0x0, > .blksz = 512, > .blocks = 1, > @@ -1848,9 +1851,24 @@ static int do_rpmb_op(int fd, > .data_ptr = (uintptr_t)frame_in > }; > + struct mmc_ioc_multi_cmd *mioc; > + struct mmc_ioc_cmd *ioc; > + > if (!frame_in || !frame_out || !out_cnt) > return -EINVAL; > + /* prepare arguments for MMC_IOC_MUTLI_CMD ioctl */ > + mioc = (struct mmc_ioc_multi_cmd *) > + malloc(sizeof (struct mmc_ioc_multi_cmd) + > + RMPB_MULTI_CMD_MAX_CMDS * sizeof (struct > mmc_ioc_cmd)); > + if (!mioc) { > + return -ENOMEM; > + } > + > + for (i = 0; i < RMPB_MULTI_CMD_MAX_CMDS; i++) { > + memcpy(&mioc->cmds[i], &ioc_base, sizeof (ioc_base)); > + } > + > rpmb_type = be16toh(frame_in->req_resp); > switch(rpmb_type) { > @@ -1861,33 +1879,24 @@ static int do_rpmb_op(int fd, > goto out; > } > + /* try to use multi_cmd if possible */ > + mioc->num_of_cmds = 3; > + > /* Write request */ > - ioc.write_flag |= (1<<31); > - err = ioctl(fd, MMC_IOC_CMD, &ioc); > - if (err < 0) { > - err = -errno; > - goto out; > - } > + ioc = &mioc->cmds[0]; > + ioc->write_flag |= (1<<31); > /* Result request */ > + ioc = &mioc->cmds[1]; > memset(frame_out, 0, sizeof(*frame_out)); > frame_out->req_resp = htobe16(MMC_RPMB_READ_RESP); > - ioc.write_flag = 1; > - ioc.data_ptr = (uintptr_t)frame_out; > - err = ioctl(fd, MMC_IOC_CMD, &ioc); > - if (err < 0) { > - err = -errno; > - goto out; > - } > + ioc->write_flag = 1; > + ioc->data_ptr = (uintptr_t)frame_out; > /* Get response */ > - ioc.write_flag = 0; > - ioc.opcode = MMC_READ_MULTIPLE_BLOCK; > - err = ioctl(fd, MMC_IOC_CMD, &ioc); > - if (err < 0) { > - err = -errno; > - goto out; > - } > + ioc = &mioc->cmds[2]; > + ioc->write_flag = 0; > + ioc->opcode = MMC_READ_MULTIPLE_BLOCK; > break; > case MMC_RPMB_READ_CNT: > @@ -1898,23 +1907,17 @@ static int do_rpmb_op(int fd, > /* fall through */ > case MMC_RPMB_READ: > - /* Request */ > - err = ioctl(fd, MMC_IOC_CMD, &ioc); > - if (err < 0) { > - err = -errno; > - goto out; > - } > + > + mioc->num_of_cmds = 2; > + > + /* Request is the same as ioc_base; don't change > mioc->cmds[0] > */ > /* Get response */ > - ioc.write_flag = 0; > - ioc.opcode = MMC_READ_MULTIPLE_BLOCK; > - ioc.blocks = out_cnt; > - ioc.data_ptr = (uintptr_t)frame_out; > - err = ioctl(fd, MMC_IOC_CMD, &ioc); > - if (err < 0) { > - err = -errno; > - goto out; > - } > + ioc = &mioc->cmds[1]; > + ioc->write_flag = 0; > + ioc->opcode = MMC_READ_MULTIPLE_BLOCK; > + ioc->blocks = out_cnt; > + ioc->data_ptr = (uintptr_t)frame_out; > break; > default: > @@ -1922,7 +1925,26 @@ static int do_rpmb_op(int fd, > goto out; > } > + /* Try to use MMC_IOC_MULTI_CMD */ > + err = ioctl(fd, MMC_IOC_MULTI_CMD, mioc); > + > + /* If MMC_IOC_MULTI_CMD is not supported, run single commands */ > + if (err < 0 && errno == EINVAL) { > + for (i = 0; i < mioc->num_of_cmds; i++) { > + err = ioctl(fd, MMC_IOC_CMD, &mioc->cmds[i]); > + if (err < 0) { > + err = -errno; > + goto out; > + } > + } > + } else if (err < 0) { > + err = -errno; > + goto out; > + } > + > + > out: > + free(mioc); > return err; > } >
Nikita hi, Thank you for fixing this long aching issue. See my comments below. Cheers, Avri > -----Original Message----- > From: linux-mmc-owner@vger.kernel.org <linux-mmc- > owner@vger.kernel.org> On Behalf Of Ulf Hansson > Sent: Monday, August 27, 2018 1:10 PM > To: Nikita Maslov <wkernelteam@gmail.com>; Chris Ball <chris@printf.net> > Cc: linux-mmc@vger.kernel.org > Subject: Re: [PATCH] mmc-utils: RPMB fails with status 0x0001 on system eMMC > chips > > + Chris > > On 27 August 2018 at 11:27, Nikita Maslov <wkernelteam@gmail.com> wrote: > > There is a problem with RPMB access on systems which use eMMC as > > default storage. > > > > According to specs, RPMB requires multiple commands to be sent > > right one after another, in another case, if there are some other > > commands between them, RPMB will fail with status 0x0001. This case > > is common for systems which use eMMC as default system storage. > > > > In current version of mmc-utils RPMB operations are performed as > > a sequence of MMC_IOC_CMD ioctl calls. Between these calls eMMC is > > unlocked and may be used by mmc driver, so RPMB session will be broken. > > > > In Linux 4.4+, there is MMC_IOC_MULTI_CMD ioctl which allows to perform > > multiple IOC_CMDs in one request. This is a better way to work with > > RPMB, because eMMC is supposed to be locked during ioctl. > > > > I included the patch which tries to use MMC_IOC_MULTI_CMD for RPMB > > operations. If it fails with EINVAL, it tries to use MMC_IOC_CMD > > (for compatibility). No need, prior to removing the host lock, MMC_IOC_MULTI_CMD was supported as well, holding that lock. > > > > This patch was tested on ARM systems (armel) with eMMC chips from > > Micron and SanDisk. The rest of the commit message is not needed Also please add Signed-off-by > > > > All suggestions are welcomed. > > > > P.S. This patch was sent to Debian bug tracking system but was left > > without reply, so I decided to send it here. > > > > -- System Information: > > Debian Release: buster/sid > > APT prefers testing > > APT policy: (900, 'testing'), (500, 'testing-debug'), (10, 'unstable') > > Architecture: amd64 (x86_64) > > Foreign Architectures: i386 > > > > Kernel: Linux 4.14.0-3-amd64 (SMP w/4 CPU cores) > > Locale: LANG=en_US.UTF-8, LC_CTYPE=en_US.UTF-8 (charmap=UTF-8), > > LANGUAGE=en_US:en (charmap=UTF-8) > > Shell: /bin/sh linked to /bin/dash > > Init: systemd (via /run/systemd/system) > > LSM: AppArmor: enabled > > > > Versions of packages mmc-utils depends on: > > ii libc6 2.26-6 > > > > mmc-utils recommends no packages. > > > > mmc-utils suggests no packages. > > > > -- no debconf information > > > > Author: Nikita Maslov <wkernelt...@gmail.com> > > Description: Use MMC_IOC_MULTI_CMD for RPMB access > > On some systems which use MMC as a main storage device > > it is possible that RPMB commands are mixed with > > generic MMC access commands which invalidates RPMB. > > This patch uses MMC_IOC_MULTI_CMD. > > . > > mmc-utils (0+git20170901.37c86e60-1) unstable; urgency=medium > > . > > * Snapshot, taken from the master (20170901). > > * Update supporting version of debhelper to 10. > > * Update Standards-Version to 4.1.0. > > > > Last-Update: 2018-03-07 > > > > --- > > > > --- mmc-utils-0+git20170901.37c86e60.orig/mmc_cmds.c > > +++ mmc-utils-0+git20170901.37c86e60/mmc_cmds.c > > @@ -1799,6 +1799,8 @@ int do_sanitize(int nargs, char **argv) > > ret; > > \ > > }) > > +#define RMPB_MULTI_CMD_MAX_CMDS 3 > > + > > enum rpmb_op_type { > > MMC_RPMB_WRITE_KEY = 0x01, > > MMC_RPMB_READ_CNT = 0x02, > > @@ -1837,8 +1839,9 @@ static int do_rpmb_op(int fd, > > { > > int err; > > u_int16_t rpmb_type; > > + unsigned i; > > - struct mmc_ioc_cmd ioc = { > > + struct mmc_ioc_cmd ioc_base = { > > .arg = 0x0, > > .blksz = 512, > > .blocks = 1, > > @@ -1848,9 +1851,24 @@ static int do_rpmb_op(int fd, > > .data_ptr = (uintptr_t)frame_in > > }; > > + struct mmc_ioc_multi_cmd *mioc; > > + struct mmc_ioc_cmd *ioc; > > + > > if (!frame_in || !frame_out || !out_cnt) > > return -EINVAL; one line space please > > + /* prepare arguments for MMC_IOC_MUTLI_CMD ioctl */ > > + mioc = (struct mmc_ioc_multi_cmd *) > > + malloc(sizeof (struct mmc_ioc_multi_cmd) + > > + RMPB_MULTI_CMD_MAX_CMDS * sizeof (struct > > mmc_ioc_cmd)); 80 characters > > + if (!mioc) { > > + return -ENOMEM; > > + } > > + > > + for (i = 0; i < RMPB_MULTI_CMD_MAX_CMDS; i++) { > > + memcpy(&mioc->cmds[i], &ioc_base, sizeof (ioc_base)); I find this option of ioc_base less clear than a static inline set_single_cmd that sets opcode and flags. This way you are actively setting what you need for each command, Instead of fixing the default settings. > > + } > > + > > rpmb_type = be16toh(frame_in->req_resp); > > switch(rpmb_type) { You may consider to withdraw from the switch-case structure - you'll end up with less code, But keeping it is fine as well. > > @@ -1861,33 +1879,24 @@ static int do_rpmb_op(int fd, > > goto out; > > } > > + /* try to use multi_cmd if possible */ > > + mioc->num_of_cmds = 3; > > + > > /* Write request */ > > - ioc.write_flag |= (1<<31); > > - err = ioctl(fd, MMC_IOC_CMD, &ioc); > > - if (err < 0) { > > - err = -errno; > > - goto out; > > - } > > + ioc = &mioc->cmds[0]; > > + ioc->write_flag |= (1<<31); > > /* Result request */ > > + ioc = &mioc->cmds[1]; > > memset(frame_out, 0, sizeof(*frame_out)); > > frame_out->req_resp = htobe16(MMC_RPMB_READ_RESP); > > - ioc.write_flag = 1; > > - ioc.data_ptr = (uintptr_t)frame_out; > > - err = ioctl(fd, MMC_IOC_CMD, &ioc); > > - if (err < 0) { > > - err = -errno; > > - goto out; > > - } > > + ioc->write_flag = 1; > > + ioc->data_ptr = (uintptr_t)frame_out; maybe use mmc_ioc_cmd_set_data? > > /* Get response */ > > - ioc.write_flag = 0; > > - ioc.opcode = MMC_READ_MULTIPLE_BLOCK; > > - err = ioctl(fd, MMC_IOC_CMD, &ioc); > > - if (err < 0) { > > - err = -errno; > > - goto out; > > - } > > + ioc = &mioc->cmds[2]; > > + ioc->write_flag = 0; > > + ioc->opcode = MMC_READ_MULTIPLE_BLOCK; > > break; > > case MMC_RPMB_READ_CNT: > > @@ -1898,23 +1907,17 @@ static int do_rpmb_op(int fd, > > /* fall through */ > > case MMC_RPMB_READ: > > - /* Request */ > > - err = ioctl(fd, MMC_IOC_CMD, &ioc); > > - if (err < 0) { > > - err = -errno; > > - goto out; > > - } > > + > > + mioc->num_of_cmds = 2; > > + > > + /* Request is the same as ioc_base; don't change > > mioc->cmds[0] > > */ 80 characters > > /* Get response */ > > - ioc.write_flag = 0; > > - ioc.opcode = MMC_READ_MULTIPLE_BLOCK; > > - ioc.blocks = out_cnt; > > - ioc.data_ptr = (uintptr_t)frame_out; > > - err = ioctl(fd, MMC_IOC_CMD, &ioc); > > - if (err < 0) { > > - err = -errno; > > - goto out; > > - } > > + ioc = &mioc->cmds[1]; > > + ioc->write_flag = 0; > > + ioc->opcode = MMC_READ_MULTIPLE_BLOCK; > > + ioc->blocks = out_cnt; > > + ioc->data_ptr = (uintptr_t)frame_out; mmc_ioc_cmd_set_data? > > break; > > default: > > @@ -1922,7 +1925,26 @@ static int do_rpmb_op(int fd, > > goto out; > > } One line space > > + /* Try to use MMC_IOC_MULTI_CMD */ No need for that comment > > + err = ioctl(fd, MMC_IOC_MULTI_CMD, mioc); > > + > > + /* If MMC_IOC_MULTI_CMD is not supported, run single commands */ This is actually not needed > > + if (err < 0 && errno == EINVAL) { > > + for (i = 0; i < mioc->num_of_cmds; i++) { > > + err = ioctl(fd, MMC_IOC_CMD, &mioc->cmds[i]); > > + if (err < 0) { > > + err = -errno; > > + goto out; > > + } > > + } > > + } else if (err < 0) { > > + err = -errno; > > + goto out; > > + } > > + > > + > > out: > > + free(mioc); > > return err; > > } > >
--- mmc-utils-0+git20170901.37c86e60.orig/mmc_cmds.c +++ mmc-utils-0+git20170901.37c86e60/mmc_cmds.c @@ -1799,6 +1799,8 @@ int do_sanitize(int nargs, char **argv) ret; \ }) +#define RMPB_MULTI_CMD_MAX_CMDS 3 + enum rpmb_op_type { MMC_RPMB_WRITE_KEY = 0x01,
There is a problem with RPMB access on systems which use eMMC as default storage. According to specs, RPMB requires multiple commands to be sent right one after another, in another case, if there are some other commands between them, RPMB will fail with status 0x0001. This case is common for systems which use eMMC as default system storage. In current version of mmc-utils RPMB operations are performed as a sequence of MMC_IOC_CMD ioctl calls. Between these calls eMMC is unlocked and may be used by mmc driver, so RPMB session will be broken. In Linux 4.4+, there is MMC_IOC_MULTI_CMD ioctl which allows to perform multiple IOC_CMDs in one request. This is a better way to work with RPMB, because eMMC is supposed to be locked during ioctl. I included the patch which tries to use MMC_IOC_MULTI_CMD for RPMB operations. If it fails with EINVAL, it tries to use MMC_IOC_CMD (for compatibility). This patch was tested on ARM systems (armel) with eMMC chips from Micron and SanDisk. All suggestions are welcomed. P.S. This patch was sent to Debian bug tracking system but was left without reply, so I decided to send it here. -- System Information: Debian Release: buster/sid APT prefers testing APT policy: (900, 'testing'), (500, 'testing-debug'), (10, 'unstable') Architecture: amd64 (x86_64) Foreign Architectures: i386 Kernel: Linux 4.14.0-3-amd64 (SMP w/4 CPU cores) Locale: LANG=en_US.UTF-8, LC_CTYPE=en_US.UTF-8 (charmap=UTF-8), LANGUAGE=en_US:en (charmap=UTF-8) Shell: /bin/sh linked to /bin/dash Init: systemd (via /run/systemd/system) LSM: AppArmor: enabled Versions of packages mmc-utils depends on: ii libc6 2.26-6 mmc-utils recommends no packages. mmc-utils suggests no packages. -- no debconf information Author: Nikita Maslov <wkernelt...@gmail.com> Description: Use MMC_IOC_MULTI_CMD for RPMB access On some systems which use MMC as a main storage device it is possible that RPMB commands are mixed with generic MMC access commands which invalidates RPMB. This patch uses MMC_IOC_MULTI_CMD. . mmc-utils (0+git20170901.37c86e60-1) unstable; urgency=medium . * Snapshot, taken from the master (20170901). * Update supporting version of debhelper to 10. * Update Standards-Version to 4.1.0. Last-Update: 2018-03-07 --- MMC_RPMB_READ_CNT = 0x02, @@ -1837,8 +1839,9 @@ static int do_rpmb_op(int fd, { int err; u_int16_t rpmb_type; + unsigned i; - struct mmc_ioc_cmd ioc = { + struct mmc_ioc_cmd ioc_base = { .arg = 0x0, .blksz = 512, .blocks = 1, @@ -1848,9 +1851,24 @@ static int do_rpmb_op(int fd, .data_ptr = (uintptr_t)frame_in }; + struct mmc_ioc_multi_cmd *mioc; + struct mmc_ioc_cmd *ioc; + if (!frame_in || !frame_out || !out_cnt) return -EINVAL; + /* prepare arguments for MMC_IOC_MUTLI_CMD ioctl */ + mioc = (struct mmc_ioc_multi_cmd *) + malloc(sizeof (struct mmc_ioc_multi_cmd) + + RMPB_MULTI_CMD_MAX_CMDS * sizeof (struct mmc_ioc_cmd)); + if (!mioc) { + return -ENOMEM; + } + + for (i = 0; i < RMPB_MULTI_CMD_MAX_CMDS; i++) { + memcpy(&mioc->cmds[i], &ioc_base, sizeof (ioc_base)); + } + rpmb_type = be16toh(frame_in->req_resp); switch(rpmb_type) { @@ -1861,33 +1879,24 @@ static int do_rpmb_op(int fd, goto out; } + /* try to use multi_cmd if possible */ + mioc->num_of_cmds = 3; + /* Write request */ - ioc.write_flag |= (1<<31); - err = ioctl(fd, MMC_IOC_CMD, &ioc); - if (err < 0) { - err = -errno; - goto out; - } + ioc = &mioc->cmds[0]; + ioc->write_flag |= (1<<31); /* Result request */ + ioc = &mioc->cmds[1]; memset(frame_out, 0, sizeof(*frame_out)); frame_out->req_resp = htobe16(MMC_RPMB_READ_RESP); - ioc.write_flag = 1; - ioc.data_ptr = (uintptr_t)frame_out; - err = ioctl(fd, MMC_IOC_CMD, &ioc); - if (err < 0) { - err = -errno; - goto out; - } + ioc->write_flag = 1; + ioc->data_ptr = (uintptr_t)frame_out; /* Get response */ - ioc.write_flag = 0; - ioc.opcode = MMC_READ_MULTIPLE_BLOCK; - err = ioctl(fd, MMC_IOC_CMD, &ioc); - if (err < 0) { - err = -errno; - goto out; - } + ioc = &mioc->cmds[2]; + ioc->write_flag = 0; + ioc->opcode = MMC_READ_MULTIPLE_BLOCK; break; case MMC_RPMB_READ_CNT: @@ -1898,23 +1907,17 @@ static int do_rpmb_op(int fd, /* fall through */ case MMC_RPMB_READ: - /* Request */ - err = ioctl(fd, MMC_IOC_CMD, &ioc); - if (err < 0) { - err = -errno; - goto out; - } + + mioc->num_of_cmds = 2; + + /* Request is the same as ioc_base; don't change mioc->cmds[0] */ /* Get response */ - ioc.write_flag = 0; - ioc.opcode = MMC_READ_MULTIPLE_BLOCK; - ioc.blocks = out_cnt; - ioc.data_ptr = (uintptr_t)frame_out; - err = ioctl(fd, MMC_IOC_CMD, &ioc); - if (err < 0) { - err = -errno; - goto out; - } + ioc = &mioc->cmds[1]; + ioc->write_flag = 0; + ioc->opcode = MMC_READ_MULTIPLE_BLOCK; + ioc->blocks = out_cnt; + ioc->data_ptr = (uintptr_t)frame_out; break; default: @@ -1922,7 +1925,26 @@ static int do_rpmb_op(int fd, goto out; } + /* Try to use MMC_IOC_MULTI_CMD */ + err = ioctl(fd, MMC_IOC_MULTI_CMD, mioc); + + /* If MMC_IOC_MULTI_CMD is not supported, run single commands */ + if (err < 0 && errno == EINVAL) { + for (i = 0; i < mioc->num_of_cmds; i++) { + err = ioctl(fd, MMC_IOC_CMD, &mioc->cmds[i]); + if (err < 0) { + err = -errno; + goto out; + } + } + } else if (err < 0) { + err = -errno; + goto out; + } + + out: + free(mioc); return err; }