On Thu, Sep 28, 2017 at 09:03:59PM +0300, Volodymyr Babchuk wrote:
> +static int
> +tee_ioctl_shm_register(struct tee_context *ctx,
> + struct tee_ioctl_shm_register_data __user *udata)
> +{
> + long ret;
> + struct tee_ioctl_shm_register_data data;
> + struct tee_shm *shm;
> +
> + if (copy_from_user(&data, udata, sizeof(data)))
> + return -EFAULT;
> +
> + /* Currently no input flags are supported */
> + if (data.flags)
> + return -EINVAL;
> +
> + shm = tee_shm_register(ctx, data.addr, data.length,
> + TEE_SHM_DMA_BUF | TEE_SHM_USER_MAPPED);
> + if (IS_ERR(shm))
> + return PTR_ERR(shm);
> +
> + data.id = shm->id;
> + data.flags = shm->flags;
> + data.length = shm->size;
> +
> + if (copy_to_user(udata, &data, sizeof(data)))
> + ret = -EFAULT;
> + else
> + ret = tee_shm_get_fd(shm);
Why do you need both the fd and an id? That seems redundant.
[...]
> +struct tee_shm *tee_shm_register(struct tee_context *ctx, unsigned long addr,
> + size_t length, u32 flags)
> +{
> + struct tee_device *teedev = ctx->teedev;
> + const u32 req_flags = TEE_SHM_DMA_BUF | TEE_SHM_USER_MAPPED;
> + struct tee_shm *shm;
> + void *ret;
> + int rc;
> + int num_pages;
> + unsigned long start;
> +
> + if (flags != req_flags) {
> + dev_err(teedev->dev.parent, "invliad shm flags %#x", flags);
> + return ERR_PTR(-EINVAL);
> + }
> +
> + if (!tee_device_get(teedev))
> + return ERR_PTR(-EINVAL);
> +
> + if (!teedev->desc->ops->shm_register ||
> + !teedev->desc->ops->shm_unregister) {
> + dev_err(teedev->dev.parent,
> + "register shared memory unspported by device");
I don't think this should be a dev_err. The user requested something
that the device did not support, but that's not a device-side error.
A user may legitmiately do this to probe whether the TEE supports
registering memory.
> + tee_device_put(teedev);
> + return ERR_PTR(-EINVAL);
Perhaps EOPNOTSUPP?
> + }
> +
> + shm = kzalloc(sizeof(*shm), GFP_KERNEL);
> + if (!shm) {
> + ret = ERR_PTR(-ENOMEM);
> + goto err;
> + }
> +
> + shm->flags = flags | TEE_SHM_REGISTER;
> + shm->teedev = teedev;
> + shm->ctx = ctx;
> + shm->id = -1;
> + start = rounddown(addr, PAGE_SIZE);
> + shm->offset = addr - start;
> + shm->size = length;
> + num_pages = (roundup(addr + length, PAGE_SIZE) - start) / PAGE_SIZE;
Why not mandate that the user passes a buffer which has a start and end
aligned to PAGE_SIZE?
Otherwise, the buffer is size is silently upgraded without the user's
knowledge, which seems likely to result in bugs.
> + shm->pages = kcalloc(num_pages, sizeof(struct page), GFP_KERNEL);
I think you mean sizeof(struct page *) here.
Generally, for:
lhs = some_alloc(sizeof(x))
... it's preferred that x is *lhs, so as to keep the types in sync. e.g.
shm->pages = kcalloc(num_pages, sizeof(*shm->pages), GFP_KERNEL);
> + if (!shm->pages) {
> + ret = ERR_PTR(-ENOMEM);
> + goto err;
> + }
> +
> + rc = get_user_pages_fast(start, num_pages, 1, shm->pages);
> + if (rc > 0)
> + shm->num_pages = rc;
> + if (rc != num_pages) {
> + if (rc > 0)
> + rc = -ENOMEM;
> + ret = ERR_PTR(rc);
> + goto err;
> + }
> +
> + mutex_lock(&teedev->mutex);
> + shm->id = idr_alloc(&teedev->idr, shm, 1, 0, GFP_KERNEL);
> + mutex_unlock(&teedev->mutex);
AFAICT, idr_alloc() can fail, so I beleive you're missing a sanity check
on the return value here.
THanks,
Mark.
