| Message ID | 20181120230832.1840-2-wsa+renesas@sang-engineering.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | mmc: refactor RPMB block count handling | expand |
> > For RPMB, block count is a non-zero 16 bit wide number. Reject invalid Actually this is not what limits the number of rpmb frames to be read, As those are not indicated in the block_count field of the rpmb frame, But in CMD23. While it is true that by eMMCv52 RPMB_SIZE_MULT max value is 0x80, Which limits the RPMB are to 16M, or 65535 rpmb frames, I don't think that it is up to the host to validate the rpmb frame content - The device will return the appropriate error should such an error occur. Also, specs are changing from time to time, so hard-coding 65535 is less appropriate. > values from userspace instead of just masking the unneeded bits. Tested > with a modified 'mmc-utils' package. > > Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com> > --- > drivers/mmc/core/block.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/drivers/mmc/core/block.c b/drivers/mmc/core/block.c > index c35b5b08bb33..9e0f7e4aa8c6 100644 > --- a/drivers/mmc/core/block.c > +++ b/drivers/mmc/core/block.c > @@ -550,6 +550,9 @@ static int __mmc_blk_ioctl_cmd(struct mmc_card > *card, struct mmc_blk_data *md, > } > > if (idata->rpmb) { > + if (data.blocks > 65535 || !data.blocks) > + return -EINVAL; > + Other than my comment above, this series looks fine to me. Thanks, Avri
Hi Avri, thanks for your comments! > > For RPMB, block count is a non-zero 16 bit wide number. Reject invalid > Actually this is not what limits the number of rpmb frames to be read, > As those are not indicated in the block_count field of the rpmb frame, > But in CMD23. > While it is true that by eMMCv52 RPMB_SIZE_MULT max value is 0x80, > Which limits the RPMB are to 16M, or 65535 rpmb frames, > I don't think that it is up to the host to validate the rpmb frame content - I wanted to fix the following issue: Currently, in to-be-removed mmc_set_blockcount() we have: cmd.arg = blockcount & 0x0000FFFF; So, sending a IOCTL with a value of 65537 will silently(!) produce a block count of 1. I didn't like this. I don't have the eMMC specs on this computer but I recall it is defined somewhere that there are 2 bytes reserved for it in the packet frame? But yes, standards may be extended, so I am not opposed to fill in bigger numbers than 16 bit and let the card handle the error. So you suggest dropping this patch and keep the others, did I get this right? Would be fine with me. Regards, Wolfram
> > Hi Avri, > > thanks for your comments! > > > > For RPMB, block count is a non-zero 16 bit wide number. Reject invalid > > Actually this is not what limits the number of rpmb frames to be read, > > As those are not indicated in the block_count field of the rpmb frame, > > But in CMD23. > > While it is true that by eMMCv52 RPMB_SIZE_MULT max value is 0x80, > > Which limits the RPMB are to 16M, or 65535 rpmb frames, > > I don't think that it is up to the host to validate the rpmb frame content - > > I wanted to fix the following issue: > > Currently, in to-be-removed mmc_set_blockcount() we have: > > cmd.arg = blockcount & 0x0000FFFF; > > So, sending a IOCTL with a value of 65537 will silently(!) produce a > block count of 1. I didn't like this. > > I don't have the eMMC specs on this computer but I recall it is defined > somewhere that there are 2 bytes reserved for it in the packet frame? Yes. But strangely as it may sound, this u16 in the rpmb frame should be set to 0x0 in all cases except for data write in which it may carry 1, 2, or 32. The block count for all other operations is set by CMD23. This might explain why the community people hate JEDEC guys so much. I agree - the blockcount in mmc_set_blockcount carries not the frame Content, but the mmc_ioc_cmd blocks field, which is unsigned int. So this line in mmc_set_blockcount performs an unnecessary input validation. > > But yes, standards may be extended, so I am not opposed to fill in > bigger numbers than 16 bit and let the card handle the error. > > So you suggest dropping this patch and keep the others, did I get this > right? Would be fine with me. Yes. Thanks. Cheers, Avri > > Regards, > > Wolfram
> This might explain why the community people hate JEDEC guys so much. :D Okay, this u16 problem of block count exists in mmc-tools, too. And I'd like to resend this series with a comment added to explain the situation. Because the specs seem to be easy to get wrong here. > So this line in mmc_set_blockcount performs an unnecessary input validation. Even wrong input validation, as I understand it. > > So you suggest dropping this patch and keep the others, did I get this > > right? Would be fine with me. > Yes. Thanks. Good, let's do this then.
On Wed, 21 Nov 2018 at 00:10, Wolfram Sang <wsa+renesas@sang-engineering.com> wrote: > > For RPMB, block count is a non-zero 16 bit wide number. Reject invalid > values from userspace instead of just masking the unneeded bits. Tested > with a modified 'mmc-utils' package. > > Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com> Tested-by: Clément Péron <peron.clem@gmail.com> > --- > drivers/mmc/core/block.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/drivers/mmc/core/block.c b/drivers/mmc/core/block.c > index c35b5b08bb33..9e0f7e4aa8c6 100644 > --- a/drivers/mmc/core/block.c > +++ b/drivers/mmc/core/block.c > @@ -550,6 +550,9 @@ static int __mmc_blk_ioctl_cmd(struct mmc_card *card, struct mmc_blk_data *md, > } > > if (idata->rpmb) { > + if (data.blocks > 65535 || !data.blocks) > + return -EINVAL; > + > err = mmc_set_blockcount(card, data.blocks, > idata->ic.write_flag & (1 << 31)); > if (err) > -- > 2.11.0 >
diff --git a/drivers/mmc/core/block.c b/drivers/mmc/core/block.c index c35b5b08bb33..9e0f7e4aa8c6 100644 --- a/drivers/mmc/core/block.c +++ b/drivers/mmc/core/block.c @@ -550,6 +550,9 @@ static int __mmc_blk_ioctl_cmd(struct mmc_card *card, struct mmc_blk_data *md, } if (idata->rpmb) { + if (data.blocks > 65535 || !data.blocks) + return -EINVAL; + err = mmc_set_blockcount(card, data.blocks, idata->ic.write_flag & (1 << 31)); if (err)
For RPMB, block count is a non-zero 16 bit wide number. Reject invalid values from userspace instead of just masking the unneeded bits. Tested with a modified 'mmc-utils' package. Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com> --- drivers/mmc/core/block.c | 3 +++ 1 file changed, 3 insertions(+)