Re: [PATCH v2 4/4] tools rpmb: add RPBM access tool

[Harald Mommer <hmo@opensynergy.com>]

Hello,

On 05.04.22 11:37, Alex Bennée wrote:
> +/*
> + * Utility functions
> + */
> +static int open_dev_file(const char *devfile, struct rpmb_ioc_cap_cmd *cap)
> +{
> ...
> +	rpmb_dbg("RPMB rpmb_target = %d\n", cap->target);
> +	rpmb_dbg("RPMB capacity    = %d\n", cap->capacity);
> +	rpmb_dbg("RPMB block_size  = %d\n", cap->block_size);
> +	rpmb_dbg("RPMB wr_cnt_max  = %d\n", cap->wr_cnt_max);
> +	rpmb_dbg("RPMB rd_cnt_max  = %d\n", cap->rd_cnt_max);
> +	rpmb_dbg("RPMB auth_method = %d\n", cap->auth_method);

The previously present device_type has already gone. Do we loose an 
opportunity to inform the user about anything which may not be virtio 
RPMB in the future or is this intentional? A new device type VIRTIO_RPMB 
instead and keeping the door open for UFS, NVMe, eMMC etc.?

And if the removal was intentional: rpmb_target, block_size, 
auth_method: Still needed and if so for what is it good for?

> ...
> +}
> +
> +static struct virtio_rpmb_frame * vrpmb_alloc_frames(int n)
> +{
> +	struct virtio_rpmb_frame *frames;
> +
> +	frames = calloc(n, sizeof(struct virtio_rpmb_frame));
> +	if (frames)
> +		memset(frames, 0, n *  sizeof(struct virtio_rpmb_frame));
Very minor: calloc() already zeroes.
> +	return frames;
> +}
> +

/*
+ * To read blocks we receive a bunch of frames from the vrpmb device
+ * which we need to validate and extract the actual data into
+ * requested buffer.
+ */
+static int vrpmb_read_blocks(int dev_fd, void *key, int addr, int count, void *data, int len)
+{
+...
+	ret = ioctl(dev_fd, RPMB_IOC_RBLOCKS_CMD, &cmd);
+	if (ret < 0) {
+		rpmb_err("rblocks ioctl failure %d: %s.\n", ret,
+			 strerror(errno));
+		goto out;
+	}

The (older) rpmb tool always puzzles me with complaining about the mismatch MAC when there was also a result != VIRTIO_RPMB_RES_OK.
I guess the ioctl() returns 0 if the ioctl() as such succeeded but there is an error code indicated in the frame.
Consider adding to print the result if it indicated a problem.

I can remember the old tool used the result of the last frame for this purpose which was probably not a good choice, not sure why this was that way, probably the first frame would be a better choice.

+	for (i = 0; i < count; i++) {
+		struct virtio_rpmb_frame *f = &frames[i];
+
+		vrpmb_dump_frame("block data", f);
+
+		if (key) {
+			ret = vrpmb_check_mac(key, f, 1);
+			if (ret) {
+				rpmb_err("%s: check mac error frame %d/%d\n", __func__, i, ret);
+				break;
+			}
+		}
+
+		memcpy(data, &f->data, RPMB_BLOCK_SIZE);
+		data += RPMB_BLOCK_SIZE;
+	}
+	ret = 0;
+
+ out:
+	free(frames);
+	return ret;
+}
+
+
+static void *read_key(const char *path)
+{
+	int key_fd = open_rd_file(path, "key file");
+	void *key;
+
+	if (key_fd < 0)
+		return NULL;
+
+	key = malloc(RPMB_KEY_SIZE);

Very minor: There's not a single free() for key in the code. Plays no 
role here as the program runs only for a fraction of a second, just saw 
it. You are making your life harder as necessary by using dynamic memory 
when it can be avoided, all those NULL pointer checks and the free(s) 
which have to be done...
> +
> +	if (!key) {
> +		rpmb_err("out of memory for key\n");
> +		return NULL;
> +	}
> +
> +	if (read(key_fd, key, RPMB_KEY_SIZE) != RPMB_KEY_SIZE) {
> +		rpmb_err("couldn't read key (%s)\n", strerror(errno));
> +		return NULL;
> +	}
> +
> +	close(key_fd);
> +	return key;
> +}
> +
>
> +
> +static const struct rpmb_cmd cmds[] = {
> +	{
> +		"get-info",
> +		op_get_info,
> +		"<RPMB_DEVICE>",
> +		"    Get RPMB device info\n",
> +	},
> +	{
> +		"program-key",
> +		op_rpmb_program_key,
> +		"<RPMB_DEVICE> <KEYFILE>",
> +		"    Program authentication KEYFILE\n"
> +		"    NOTE: This is a one-time programmable irreversible change.\n",
> +	},
> +	{
> +		"write-counter",
> +		op_rpmb_get_write_counter,
> +		"<RPMB_DEVICE>",
> +		"    Rertrive write counter value from the <RPMB_DEVICE> to stdout.\n"

Optional parameter explanation [KEYFILE] got lost in write-counter.

> +	},
> +	{
> +		"write-blocks",
> +		op_rpmb_write_blocks,
> +		"<RPMB_DEVICE> <address> <block_count> <DATA_FILE> <KEYID>",
> +		"    <block count> of 256 bytes will be written from the DATA_FILE\n"
> +		"    to the <RPMB_DEVICE> at block offset <address>.\n"
> +		"    When DATA_FILE is -, read from standard input.\n",
Puzzling naming change. The KEYID is indeed the <KEYFILE>.
> +	},
> +	{
> +		"read-blocks",
> +		op_rpmb_read_blocks,
> +		"<RPMB_DEVICE> <address> <blocks count> <OUTPUT_FILE>",
> +		"    <block count> of 256 bytes will be read from <RPMB_DEVICE>\n"
> +		"    to the OUTPUT_FILE\n"
> +		"    When OUTPUT_FILE is -, write to standard output\n",

Here also the optional parameter explanation [KEYFILE] got lost in 
read-blocks.

For people not knowing the tool trying to get into RPMB a complete and 
consistent help is helpful I can still remember when I was in the 
situation to understand more playing around with the (older) tool.

> +	},
> +
> +	{ NULL, NULL, NULL, NULL }
> +};

-- 
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