linux-kernel - Re: [PATCH v2 3/4] rpmb: create virtio rpmb frontend driver

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <b0af6903-6847-93ed-5561-1c43e84ef7e9@opensynergy.com>
Date:   Thu, 16 Jun 2022 17:44:04 +0200
From:   Harald Mommer <hmo@...nsynergy.com>
To:     Alex Bennée <alex.bennee@...aro.org>,
        linux-kernel@...r.kernel.org
Cc:     maxim.uvarov@...aro.org, joakim.bech@...aro.org,
        ulf.hansson@...aro.org, ilias.apalodimas@...aro.org,
        arnd@...aro.org, ruchika.gupta@...aro.org, tomas.winkler@...el.com,
        yang.huang@...el.com, bing.zhu@...el.com,
        Matti.Moell@...nsynergy.com, linux-mmc@...r.kernel.org,
        linux-scsi@...r.kernel.org
Subject: Re: [PATCH v2 3/4] rpmb: create virtio rpmb frontend driver


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@...nsynergy.com

www.opensynergy.com

Handelsregister: Amtsgericht Charlottenburg, HRB 108616B
Geschäftsführer/Managing Director: Regis Adjamah