| 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: <d963ebfb-18fb-efd7-f262-c50769ac4cbe@gmail.com>
Date: Tue, 10 Apr 2018 12:59:04 +0300
From: Oleksandr Andrushchenko <andr2000@...il.com>
To: Dongwon Kim <dongwon.kim@...el.com>, linux-kernel@...r.kernel.org,
linaro-mm-sig@...ts.linaro.org, xen-devel@...ts.xenproject.org
Cc: dri-devel@...ts.freedesktop.org, mateuszx.potrola@...el.com
Subject: Re: [RFC, v2, 4/9] hyper_dmabuf: user private data attached to
hyper_DMABUF
On 02/14/2018 03:50 AM, Dongwon Kim wrote:
> Define a private data (e.g. meta data for the buffer) attached to
> each hyper_DMABUF structure. This data is provided by userapace via
> export_remote IOCTL and its size can be up to 192 bytes.
>
> Signed-off-by: Dongwon Kim <dongwon.kim@...el.com>
> Signed-off-by: Mateusz Polrola <mateuszx.potrola@...el.com>
> ---
> drivers/dma-buf/hyper_dmabuf/hyper_dmabuf_ioctl.c | 83 ++++++++++++++++++++--
> drivers/dma-buf/hyper_dmabuf/hyper_dmabuf_msg.c | 36 +++++++++-
> drivers/dma-buf/hyper_dmabuf/hyper_dmabuf_msg.h | 2 +-
> .../dma-buf/hyper_dmabuf/hyper_dmabuf_sgl_proc.c | 1 +
> drivers/dma-buf/hyper_dmabuf/hyper_dmabuf_struct.h | 12 ++++
> include/uapi/linux/hyper_dmabuf.h | 4 ++
> 6 files changed, 132 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/dma-buf/hyper_dmabuf/hyper_dmabuf_ioctl.c b/drivers/dma-buf/hyper_dmabuf/hyper_dmabuf_ioctl.c
> index 020a5590a254..168ccf98f710 100644
> --- a/drivers/dma-buf/hyper_dmabuf/hyper_dmabuf_ioctl.c
> +++ b/drivers/dma-buf/hyper_dmabuf/hyper_dmabuf_ioctl.c
> @@ -103,6 +103,11 @@ static int send_export_msg(struct exported_sgt_info *exported,
> }
> }
>
> + op[8] = exported->sz_priv;
> +
> + /* driver/application specific private info */
> + memcpy(&op[9], exported->priv, op[8]);
> +
> req = kcalloc(1, sizeof(*req), GFP_KERNEL);
>
> if (!req)
> @@ -120,8 +125,9 @@ static int send_export_msg(struct exported_sgt_info *exported,
>
> /* Fast path exporting routine in case same buffer is already exported.
> *
> - * If same buffer is still valid and exist in EXPORT LIST it returns 0 so
> - * that remaining normal export process can be skipped.
> + * If same buffer is still valid and exist in EXPORT LIST, it only updates
> + * user-private data for the buffer and returns 0 so that that it can skip
> + * normal export process.
> *
> * If "unexport" is scheduled for the buffer, it cancels it since the buffer
> * is being re-exported.
> @@ -129,7 +135,7 @@ static int send_export_msg(struct exported_sgt_info *exported,
> * return '1' if reexport is needed, return '0' if succeeds, return
> * Kernel error code if something goes wrong
> */
> -static int fastpath_export(hyper_dmabuf_id_t hid)
> +static int fastpath_export(hyper_dmabuf_id_t hid, int sz_priv, char *priv)
> {
> int reexport = 1;
> int ret = 0;
> @@ -155,6 +161,46 @@ static int fastpath_export(hyper_dmabuf_id_t hid)
> exported->unexport_sched = false;
> }
>
> + /* if there's any change in size of private data.
> + * we reallocate space for private data with new size
> + */
> + if (sz_priv != exported->sz_priv) {
> + kfree(exported->priv);
> +
> + /* truncating size */
> + if (sz_priv > MAX_SIZE_PRIV_DATA)
> + exported->sz_priv = MAX_SIZE_PRIV_DATA;
> + else
> + exported->sz_priv = sz_priv;
> +
> + exported->priv = kcalloc(1, exported->sz_priv,
> + GFP_KERNEL);
> +
> + if (!exported->priv) {
> + hyper_dmabuf_remove_exported(exported->hid);
> + hyper_dmabuf_cleanup_sgt_info(exported, true);
> + kfree(exported);
> + return -ENOMEM;
> + }
> + }
> +
> + /* update private data in sgt_info with new ones */
> + ret = copy_from_user(exported->priv, priv, exported->sz_priv);
> + if (ret) {
> + dev_err(hy_drv_priv->dev,
> + "Failed to load a new private data\n");
> + ret = -EINVAL;
> + } else {
> + /* send an export msg for updating priv in importer */
> + ret = send_export_msg(exported, NULL);
> +
> + if (ret < 0) {
> + dev_err(hy_drv_priv->dev,
> + "Failed to send a new private data\n");
> + ret = -EBUSY;
> + }
> + }
> +
> return ret;
> }
>
> @@ -191,7 +237,8 @@ static int hyper_dmabuf_export_remote_ioctl(struct file *filp, void *data)
> export_remote_attr->remote_domain);
>
> if (hid.id != -1) {
> - ret = fastpath_export(hid);
> + ret = fastpath_export(hid, export_remote_attr->sz_priv,
> + export_remote_attr->priv);
>
> /* return if fastpath_export succeeds or
> * gets some fatal error
> @@ -225,6 +272,24 @@ static int hyper_dmabuf_export_remote_ioctl(struct file *filp, void *data)
> goto fail_sgt_info_creation;
> }
>
> + /* possible truncation */
> + if (export_remote_attr->sz_priv > MAX_SIZE_PRIV_DATA)
> + exported->sz_priv = MAX_SIZE_PRIV_DATA;
> + else
> + exported->sz_priv = export_remote_attr->sz_priv;
> +
> + /* creating buffer for private data of buffer */
> + if (exported->sz_priv != 0) {
> + exported->priv = kcalloc(1, exported->sz_priv, GFP_KERNEL);
> +
> + if (!exported->priv) {
> + ret = -ENOMEM;
> + goto fail_priv_creation;
> + }
> + } else {
> + dev_err(hy_drv_priv->dev, "size is 0\n");
> + }
> +
> exported->hid = hyper_dmabuf_get_hid();
>
> /* no more exported dmabuf allowed */
> @@ -279,6 +344,10 @@ static int hyper_dmabuf_export_remote_ioctl(struct file *filp, void *data)
> INIT_LIST_HEAD(&exported->va_kmapped->list);
> INIT_LIST_HEAD(&exported->va_vmapped->list);
>
> + /* copy private data to sgt_info */
> + ret = copy_from_user(exported->priv, export_remote_attr->priv,
> + exported->sz_priv);
> +
> if (ret) {
> dev_err(hy_drv_priv->dev,
> "failed to load private data\n");
> @@ -337,6 +406,9 @@ static int hyper_dmabuf_export_remote_ioctl(struct file *filp, void *data)
>
> fail_map_active_attached:
> kfree(exported->active_sgts);
> + kfree(exported->priv);
> +
> +fail_priv_creation:
> kfree(exported);
>
> fail_map_active_sgts:
> @@ -567,6 +639,9 @@ static void delayed_unexport(struct work_struct *work)
> /* register hyper_dmabuf_id to the list for reuse */
> hyper_dmabuf_store_hid(exported->hid);
>
> + if (exported->sz_priv > 0 && !exported->priv)
> + kfree(exported->priv);
> +
> kfree(exported);
> }
> }
> diff --git a/drivers/dma-buf/hyper_dmabuf/hyper_dmabuf_msg.c b/drivers/dma-buf/hyper_dmabuf/hyper_dmabuf_msg.c
> index 129b2ff2af2b..7176fa8fb139 100644
> --- a/drivers/dma-buf/hyper_dmabuf/hyper_dmabuf_msg.c
> +++ b/drivers/dma-buf/hyper_dmabuf/hyper_dmabuf_msg.c
> @@ -60,9 +60,12 @@ void hyper_dmabuf_create_req(struct hyper_dmabuf_req *req,
> * op5 : offset of data in the first page
> * op6 : length of data in the last page
> * op7 : top-level reference number for shared pages
> + * op8 : size of private data (from op9)
> + * op9 ~ : Driver-specific private data
> + * (e.g. graphic buffer's meta info)
> */
>
> - memcpy(&req->op[0], &op[0], 8 * sizeof(int) + op[8]);
> + memcpy(&req->op[0], &op[0], 9 * sizeof(int) + op[8]);
> break;
>
> case HYPER_DMABUF_NOTIFY_UNEXPORT:
> @@ -116,6 +119,9 @@ static void cmd_process_work(struct work_struct *work)
> * op5 : offset of data in the first page
> * op6 : length of data in the last page
> * op7 : top-level reference number for shared pages
> + * op8 : size of private data (from op9)
> + * op9 ~ : Driver-specific private data
> + * (e.g. graphic buffer's meta info)
> */
>
> /* if nents == 0, it means it is a message only for
> @@ -135,6 +141,24 @@ static void cmd_process_work(struct work_struct *work)
> break;
> }
>
> + /* if size of new private data is different,
> + * we reallocate it.
> + */
> + if (imported->sz_priv != req->op[8]) {
> + kfree(imported->priv);
> + imported->sz_priv = req->op[8];
> + imported->priv = kcalloc(1, req->op[8],
> + GFP_KERNEL);
> + if (!imported->priv) {
> + /* set it invalid */
> + imported->valid = 0;
> + break;
> + }
> + }
> +
> + /* updating priv data */
> + memcpy(imported->priv, &req->op[9], req->op[8]);
> +
> break;
> }
>
> @@ -143,6 +167,14 @@ static void cmd_process_work(struct work_struct *work)
> if (!imported)
> break;
>
> + imported->sz_priv = req->op[8];
> + imported->priv = kcalloc(1, req->op[8], GFP_KERNEL);
BTW, there are plenty of the code using kcalloc with 1 element
Why not simply kzalloc?
> +
> + if (!imported->priv) {
> + kfree(imported);
> + break;
> + }
> +
> imported->hid.id = req->op[0];
>
> for (i = 0; i < 3; i++)
> @@ -162,6 +194,8 @@ static void cmd_process_work(struct work_struct *work)
> dev_dbg(hy_drv_priv->dev, "\tlast len %d\n", req->op[6]);
> dev_dbg(hy_drv_priv->dev, "\tgrefid %d\n", req->op[7]);
>
> + memcpy(imported->priv, &req->op[9], req->op[8]);
> +
> imported->valid = true;
> hyper_dmabuf_register_imported(imported);
>
> diff --git a/drivers/dma-buf/hyper_dmabuf/hyper_dmabuf_msg.h b/drivers/dma-buf/hyper_dmabuf/hyper_dmabuf_msg.h
> index 59f1528e9b1e..63a39d068d69 100644
> --- a/drivers/dma-buf/hyper_dmabuf/hyper_dmabuf_msg.h
> +++ b/drivers/dma-buf/hyper_dmabuf/hyper_dmabuf_msg.h
> @@ -27,7 +27,7 @@
> #ifndef __HYPER_DMABUF_MSG_H__
> #define __HYPER_DMABUF_MSG_H__
>
> -#define MAX_NUMBER_OF_OPERANDS 8
> +#define MAX_NUMBER_OF_OPERANDS 64
>
So now the req/resp below become (64 + 3) ints long, 268 bytes
4096 / 268...
> struct hyper_dmabuf_req {
> unsigned int req_id;
> diff --git a/drivers/dma-buf/hyper_dmabuf/hyper_dmabuf_sgl_proc.c b/drivers/dma-buf/hyper_dmabuf/hyper_dmabuf_sgl_proc.c
> index d92ae13d8a30..9032f89e0cd0 100644
> --- a/drivers/dma-buf/hyper_dmabuf/hyper_dmabuf_sgl_proc.c
> +++ b/drivers/dma-buf/hyper_dmabuf/hyper_dmabuf_sgl_proc.c
> @@ -251,6 +251,7 @@ int hyper_dmabuf_cleanup_sgt_info(struct exported_sgt_info *exported,
> kfree(exported->active_attached);
> kfree(exported->va_kmapped);
> kfree(exported->va_vmapped);
> + kfree(exported->priv);
>
> return 0;
> }
> diff --git a/drivers/dma-buf/hyper_dmabuf/hyper_dmabuf_struct.h b/drivers/dma-buf/hyper_dmabuf/hyper_dmabuf_struct.h
> index 144e3821fbc2..a1220bbf8d0c 100644
> --- a/drivers/dma-buf/hyper_dmabuf/hyper_dmabuf_struct.h
> +++ b/drivers/dma-buf/hyper_dmabuf/hyper_dmabuf_struct.h
> @@ -101,6 +101,12 @@ struct exported_sgt_info {
> * the buffer can be completely freed.
> */
> struct file *filp;
> +
> + /* size of private */
> + size_t sz_priv;
> +
> + /* private data associated with the exported buffer */
> + char *priv;
> };
>
> /* imported_sgt_info contains information about imported DMA_BUF
> @@ -126,6 +132,12 @@ struct imported_sgt_info {
> void *refs_info;
> bool valid;
> int importers;
> +
> + /* size of private */
> + size_t sz_priv;
> +
> + /* private data associated with the exported buffer */
> + char *priv;
> };
>
> #endif /* __HYPER_DMABUF_STRUCT_H__ */
> diff --git a/include/uapi/linux/hyper_dmabuf.h b/include/uapi/linux/hyper_dmabuf.h
> index caaae2da9d4d..36794a4af811 100644
> --- a/include/uapi/linux/hyper_dmabuf.h
> +++ b/include/uapi/linux/hyper_dmabuf.h
> @@ -25,6 +25,8 @@
> #ifndef __LINUX_PUBLIC_HYPER_DMABUF_H__
> #define __LINUX_PUBLIC_HYPER_DMABUF_H__
>
> +#define MAX_SIZE_PRIV_DATA 192
> +
> typedef struct {
> int id;
> int rng_key[3]; /* 12bytes long random number */
> @@ -56,6 +58,8 @@ struct ioctl_hyper_dmabuf_export_remote {
> int remote_domain;
> /* exported dma buf id */
> hyper_dmabuf_id_t hid;
> + int sz_priv;
> + char *priv;
> };
>
> #define IOCTL_HYPER_DMABUF_EXPORT_FD \
>
Powered by blists - more mailing lists