Messages in this thread | | | Date | Thu, 16 Jun 2022 17:44:04 +0200 | Subject | Re: [PATCH v2 3/4] rpmb: create virtio rpmb frontend driver | From | Harald Mommer <> |
| |
On 05.04.22 11:37, Alex Bennée wrote: > +++ b/drivers/rpmb/virtio_rpmb.c > ... > +static int rpmb_virtio_write_blocks(struct device *dev, u8 target, > + int len, u8 *req, int rlen, u8 *resp) > +{ > + struct virtio_rpmb_info *vi = dev_get_drvdata(dev); > + struct virtio_rpmb_frame *request = (struct virtio_rpmb_frame *) req; > + struct virtio_rpmb_frame *response = (struct virtio_rpmb_frame *) resp; > + struct scatterlist out_frame; > + struct scatterlist in_frame; > + struct scatterlist *sgs[2]; > + int blocks, data_len, received; > + > + if (!len || (len % VIRTIO_RPMB_FRAME_SZ) != 0 || !request) > + return -EINVAL; > + > + /* The first frame will contain the details of the request */ > + if (be16_to_cpu(request->req_resp) != VIRTIO_RPMB_REQ_DATA_WRITE) > + return -EINVAL; > + > + blocks = be16_to_cpu(request->block_count); > + if (blocks > vi->max_wr) > + return -EINVAL;
Not exactly. The virtio specification reserves 0 for "unlimited". And I see no special handling for 0 in your code. Damned trap in the specification.
What's "unlimited"? As the blocks_count in the rpmb frame is a be16 I guess it's 65535. But if you consider this theoretically you have a possible max array allocation of 16 MB - 512B max. instead of the 16 KB - 512B you have with 255. Getting headache about this "unlimited" in the virtio specification. I don't like the theoretical possibility having to allocate 16MB dynamically for a moment due to this "unlimited".
And of course there is the same problem in virtio_rpmb_read_blocks() with max_rd.
A fix is needed, either 0 is 65535 or 0 is 255. Choosing 255 as implementation decision would be limiting down but maybe without any practical impact. For UFS it's a single byte bRPMBReadWriteSize only in JESD220C-2.2 GEOMETRY DESCRIPTOR and 0 is not defined there as "unlimited". Unfortunately I've no idea about the other possible underlying technologies (eMMC, NVMe, ...).
> ... > +} > +... > +static int rpmb_virtio_read_blocks(struct device *dev, u8 target, > + int addr, int count, int len, u8 *data) > +{ > ... > + if (count > vi->max_rd) > + return -EINVAL; See above. > ...
-- Dipl.-Ing. Harald Mommer Senior Software Engineer
OpenSynergy GmbH Rotherstr. 20, 10245 Berlin
Phone: +49 (30) 60 98 540-0 <== Zentrale Fax: +49 (30) 60 98 540-99 E-Mail: harald.mommer@opensynergy.com
www.opensynergy.com
Handelsregister: Amtsgericht Charlottenburg, HRB 108616B Geschäftsführer/Managing Director: Regis Adjamah
| |