| 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
| ||
|
Message-ID:
<DM6PR04MB657580885C16AE4F5743357EFC022@DM6PR04MB6575.namprd04.prod.outlook.com>
Date: Sat, 6 Apr 2024 11:22:14 +0000
From: Avri Altman <Avri.Altman@....com>
To: Jens Wiklander <jens.wiklander@...aro.org>, "linux-kernel@...r.kernel.org"
<linux-kernel@...r.kernel.org>, "linux-mmc@...r.kernel.org"
<linux-mmc@...r.kernel.org>, "op-tee@...ts.trustedfirmware.org"
<op-tee@...ts.trustedfirmware.org>
CC: Shyam Saini <shyamsaini@...ux.microsoft.com>, Ulf Hansson
<ulf.hansson@...aro.org>, Linus Walleij <linus.walleij@...aro.org>, Jerome
Forissier <jerome.forissier@...aro.org>, Sumit Garg <sumit.garg@...aro.org>,
Ilias Apalodimas <ilias.apalodimas@...aro.org>, Bart Van Assche
<bvanassche@....org>, Randy Dunlap <rdunlap@...radead.org>, Ard Biesheuvel
<ardb@...nel.org>, Arnd Bergmann <arnd@...db.de>, Greg Kroah-Hartman
<gregkh@...uxfoundation.org>, Tomas Winkler <tomas.winkler@...el.com>,
Alexander Usyskin <alexander.usyskin@...el.com>
Subject: RE: [PATCH v4 2/3] mmc: block: register RPMB partition with the RPMB
subsystem
> +
> +#define RPMB_PROGRAM_KEY 0x1 /* Program RPMB Authentication
You expect the key to be already programmed - so you don't really need this operation?
> Key */
> +#define RPMB_GET_WRITE_COUNTER 0x2 /* Read RPMB write counter */
> +#define RPMB_WRITE_DATA 0x3 /* Write data to RPMB partition */
> +#define RPMB_READ_DATA 0x4 /* Read data from RPMB partition */
> +#define RPMB_RESULT_READ 0x5 /* Read result request (Internal) */
> +
> static DEFINE_MUTEX(block_mutex);
>
> /*
> @@ -163,6 +205,7 @@ struct mmc_rpmb_data {
> int id;
> unsigned int part_index;
> struct mmc_blk_data *md;
> + struct rpmb_dev *rdev;
> struct list_head node;
> };
>
> @@ -2672,7 +2715,6 @@ static int mmc_rpmb_chrdev_open(struct inode
> *inode, struct file *filp)
>
> get_device(&rpmb->dev);
> filp->private_data = rpmb;
> - mmc_blk_get(rpmb->md->disk);
Maybe add a comment that this has moved to mmc_blk_alloc_rpmb_part?
For those who will look for it.
>
> return nonseekable_open(inode, filp); } @@ -2682,7 +2724,6 @@ static
> int mmc_rpmb_chrdev_release(struct inode *inode, struct file *filp)
> struct mmc_rpmb_data *rpmb = container_of(inode->i_cdev,
> struct mmc_rpmb_data, chrdev);
>
> - mmc_blk_put(rpmb->md);
Ditto.
> put_device(&rpmb->dev);
>
> return 0;
> @@ -2703,10 +2744,157 @@ static void mmc_blk_rpmb_device_release(struct
> device *dev) {
> struct mmc_rpmb_data *rpmb = dev_get_drvdata(dev);
>
> + rpmb_dev_unregister(rpmb->rdev);
> + mmc_blk_put(rpmb->md);
> ida_simple_remove(&mmc_rpmb_ida, rpmb->id);
> kfree(rpmb);
> }
>
> +static void free_idata(struct mmc_blk_ioc_data **idata, unsigned int
> +cmd_count) {
> + unsigned int n;
> +
> + for (n = 0; n < cmd_count; n++)
> + kfree(idata[n]);
> + kfree(idata);
> +}
> +
> +static struct mmc_blk_ioc_data **alloc_idata(struct mmc_rpmb_data *rpmb,
> + unsigned int cmd_count) {
> + struct mmc_blk_ioc_data **idata;
> + unsigned int n;
> +
> + idata = kcalloc(cmd_count, sizeof(*idata), GFP_KERNEL);
> + if (!idata)
> + return NULL;
> +
> + for (n = 0; n < cmd_count; n++) {
> + idata[n] = kcalloc(1, sizeof(**idata), GFP_KERNEL);
> + if (!idata[n]) {
> + free_idata(idata, n);
> + return NULL;
> + }
> + idata[n]->rpmb = rpmb;
> + }
> +
> + return idata;
> +}
> +
> +static void set_idata(struct mmc_blk_ioc_data *idata, u32 opcode,
> + int write_flag, u8 *buf, unsigned int buf_bytes) {
> + idata->ic.opcode = opcode;
> + idata->ic.flags = MMC_RSP_R1 | MMC_CMD_ADTC;
> + idata->ic.write_flag = write_flag;
> + idata->ic.blksz = sizeof(struct rpmb_frame);
blksz = 512, so maybe add a compile-time check on sizeof(struct rpmb_frame)?
> + idata->ic.blocks = buf_bytes / idata->ic.blksz;
> + idata->buf = buf;
> + idata->buf_bytes = buf_bytes;
> +}
> +
> +static int mmc_route_rpmb_frames(struct device *dev, u8 *req,
> + unsigned int req_len, u8 *resp,
> + unsigned int resp_len) {
> + struct rpmb_frame *frm = (struct rpmb_frame *)req;
> + struct mmc_rpmb_data *rpmb = dev_get_drvdata(dev);
> + struct mmc_blk_data *md = rpmb->md;
> + struct mmc_blk_ioc_data **idata;
> + unsigned int cmd_count;
> + struct request *rq;
> + u16 req_type;
> + bool write;
> + int ret;
> +
> + if (IS_ERR(md->queue.card))
> + return PTR_ERR(md->queue.card);
> +
> + if (req_len < sizeof(*frm))
> + return -EINVAL;
> +
> + req_type = be16_to_cpu(frm->req_resp);
> + switch (req_type) {
> + case RPMB_PROGRAM_KEY:
> + if (req_len != sizeof(struct rpmb_frame) ||
> + resp_len != sizeof(struct rpmb_frame))
> + return -EINVAL;
> + write = true;
> + break;
> + case RPMB_GET_WRITE_COUNTER:
> + if (req_len != sizeof(struct rpmb_frame) ||
> + resp_len != sizeof(struct rpmb_frame))
> + return -EINVAL;
> + write = false;
> + break;
> + case RPMB_WRITE_DATA:
> + if (req_len % sizeof(struct rpmb_frame) ||
> + resp_len != sizeof(struct rpmb_frame))
> + return -EINVAL;
> + write = true;
> + break;
> + case RPMB_READ_DATA:
> + if (req_len != sizeof(struct rpmb_frame) ||
> + resp_len % sizeof(struct rpmb_frame))
> + return -EINVAL;
> + write = false;
> + break;
> + default:
> + return -EINVAL;
> + }
Looks like the above input validation section can be reduced to is RPMB_WRITE_DATA and default?
> +
> + if (write)
> + cmd_count = 3;
> + else
> + cmd_count = 2;
> +
> + idata = alloc_idata(rpmb, cmd_count);
> + if (!idata)
> + return -ENOMEM;
> +
> + if (write) {
> + struct rpmb_frame *frm = (struct rpmb_frame *)resp;
> +
> + /* Send write request frame(s) */
> + set_idata(idata[0], MMC_WRITE_MULTIPLE_BLOCK,
> + 1 | MMC_CMD23_ARG_REL_WR, req, req_len);
> +
> + /* Send result request frame */
> + memset(frm, 0, sizeof(*frm));
> + frm->req_resp = cpu_to_be16(RPMB_RESULT_READ);
> + set_idata(idata[1], MMC_WRITE_MULTIPLE_BLOCK, 1, resp,
> + resp_len);
> +
> + /* Read response frame */
> + set_idata(idata[2], MMC_READ_MULTIPLE_BLOCK, 0, resp,
> resp_len);
It is confusing to me that your response is holding 2 frame types:
The status frame and the response frame.
> + } else {
> + /* Send write request frame(s) */
> + set_idata(idata[0], MMC_WRITE_MULTIPLE_BLOCK, 1, req,
> + req_len);
> +
> + /* Read response frame */
> + set_idata(idata[1], MMC_READ_MULTIPLE_BLOCK, 0, resp,
> resp_len);
> + }
> +
> + rq = blk_mq_alloc_request(md->queue.queue, REQ_OP_DRV_OUT, 0);
> + if (IS_ERR(rq)) {
> + ret = PTR_ERR(rq);
> + goto out;
> + }
> +
> + req_to_mmc_queue_req(rq)->drv_op = MMC_DRV_OP_IOCTL_RPMB;
> + req_to_mmc_queue_req(rq)->drv_op_result = -EIO;
> + req_to_mmc_queue_req(rq)->drv_op_data = idata;
> + req_to_mmc_queue_req(rq)->ioc_count = cmd_count;
Maybe have an additional struct mmc_queue_req *mq_rq = req_to_mmc_queue_req(rq);
> + blk_execute_rq(rq, false);
> + ret = req_to_mmc_queue_req(rq)->drv_op_result;
> +
> + blk_mq_free_request(rq);
> +
> +out:
> + free_idata(idata, cmd_count);
> + return ret;
> +}
> +
> static int mmc_blk_alloc_rpmb_part(struct mmc_card *card,
> struct mmc_blk_data *md,
> unsigned int part_index, @@ -2741,6 +2929,7 @@ static
> int mmc_blk_alloc_rpmb_part(struct mmc_card *card,
> rpmb->dev.release = mmc_blk_rpmb_device_release;
> device_initialize(&rpmb->dev);
> dev_set_drvdata(&rpmb->dev, rpmb);
> + mmc_blk_get(md->disk);
> rpmb->md = md;
>
> cdev_init(&rpmb->chrdev, &mmc_rpmb_fileops); @@ -3002,6 +3191,41
> @@ static void mmc_blk_remove_debugfs(struct mmc_card *card,
>
> #endif /* CONFIG_DEBUG_FS */
>
> +static void mmc_blk_rpmb_add(struct mmc_card *card) {
> + struct mmc_blk_data *md = dev_get_drvdata(&card->dev);
> + struct mmc_rpmb_data *rpmb;
> + struct rpmb_dev *rdev;
> + unsigned int n;
> + u32 cid[4];
> + struct rpmb_descr descr = {
> + .type = RPMB_TYPE_EMMC,
> + .route_frames = mmc_route_rpmb_frames,
> + .reliable_wr_count = card->ext_csd.raw_rpmb_size_mult,
> + .capacity = card->ext_csd.rel_sectors,
The capacity is RPMB_SIZE_MULT and also limited to 16MB?
And you also need the region size you are writing to.
If I get your intention regarding reliable_wr_count, AFAIK, rpmb can be written either as a single, double, or 32 frames.
And this should be induced from card->ext_csd.rel_param, and not card->ext_csd.rel_sectors.
> + .dev_id = (void *)cid,
> + .dev_id_len = sizeof(cid),
> + };
> +
> + /*
> + * Provice CID as an octet array. The CID needs to be interpreted
> + * when used as input to derive the RPMB key since some fields
> + * will change due to firmware updates.
> + */
Not sure how the CID register is related to RPMB?
Is this something internal to TEE?
> + for (n = 0; n < 4; n++)
> + cid[n] = be32_to_cpu(card->raw_cid[n]);
> +
> + list_for_each_entry(rpmb, &md->rpmbs, node) {
> + rdev = rpmb_dev_register(&rpmb->dev, &descr);
> + if (IS_ERR(rdev)) {
> + pr_warn("%s: could not register RPMB device\n",
> + dev_name(&rpmb->dev));
> + continue;
> + }
> + rpmb->rdev = rdev;
> + }
> +}
> +
Thanks,
Avri
Powered by blists - more mailing lists