[bug report] drm/xe: missing access_ok() in ioctl

[Dan Carpenter]

Hello Matthew Brost,

The patch dd08ebf6c352: "drm/xe: Introduce a new DRM driver for Intel
GPUs" from Mar 30, 2023 (linux-next), leads to the following Smatch
static checker warning:

	drivers/gpu/drm/xe/xe_exec.c:163 xe_exec_ioctl()
	warn: calling '__copy_from_user()' without access_ok()

drivers/gpu/drm/xe/xe_exec.c
    102 int xe_exec_ioctl(struct drm_device *dev, void *data, struct drm_file *file)
    103 {
    104         struct xe_device *xe = to_xe_device(dev);
    105         struct xe_file *xef = to_xe_file(file);
    106         struct drm_xe_exec *args = data;
    107         struct drm_xe_sync __user *syncs_user = u64_to_user_ptr(args->syncs);
    108         u64 __user *addresses_user = u64_to_user_ptr(args->address);
    109         struct xe_exec_queue *q;
    110         struct xe_sync_entry *syncs = NULL;
    111         u64 addresses[XE_HW_ENGINE_MAX_INSTANCE];
    112         struct drm_gpuvm_exec vm_exec = {.extra.fn = xe_exec_fn};
    113         struct drm_exec *exec = &vm_exec.exec;
    114         u32 i, num_syncs = 0;
    115         struct xe_sched_job *job;
    116         struct dma_fence *rebind_fence;
    117         struct xe_vm *vm;
    118         bool write_locked;
    119         ktime_t end = 0;
    120         int err = 0;
    121 
    122         if (XE_IOCTL_DBG(xe, args->extensions) ||
    123             XE_IOCTL_DBG(xe, args->pad[0] || args->pad[1] || args->pad[2]) ||
    124             XE_IOCTL_DBG(xe, args->reserved[0] || args->reserved[1]))
    125                 return -EINVAL;
    126 
    127         q = xe_exec_queue_lookup(xef, args->exec_queue_id);
    128         if (XE_IOCTL_DBG(xe, !q))
    129                 return -ENOENT;
    130 
    131         if (XE_IOCTL_DBG(xe, q->flags & EXEC_QUEUE_FLAG_VM))
    132                 return -EINVAL;
    133 
    134         if (XE_IOCTL_DBG(xe, args->num_batch_buffer &&
    135                          q->width != args->num_batch_buffer))
    136                 return -EINVAL;
    137 
    138         if (XE_IOCTL_DBG(xe, q->flags & EXEC_QUEUE_FLAG_BANNED)) {
    139                 err = -ECANCELED;
    140                 goto err_exec_queue;
    141         }
    142 
    143         if (args->num_syncs) {
    144                 syncs = kcalloc(args->num_syncs, sizeof(*syncs), GFP_KERNEL);
    145                 if (!syncs) {
    146                         err = -ENOMEM;
    147                         goto err_exec_queue;
    148                 }
    149         }
    150 
    151         vm = q->vm;
    152 
    153         for (i = 0; i < args->num_syncs; i++) {
    154                 err = xe_sync_entry_parse(xe, xef, &syncs[num_syncs++],
    155                                           &syncs_user[i], SYNC_PARSE_FLAG_EXEC |
    156                                           (xe_vm_in_lr_mode(vm) ?
    157                                            SYNC_PARSE_FLAG_LR_MODE : 0));
    158                 if (err)
    159                         goto err_syncs;
    160         }
    161 
    162         if (xe_exec_queue_is_parallel(q)) {
--> 163                 err = __copy_from_user(addresses, addresses_user, sizeof(u64) *
    164                                        q->width);

It's been a long time since I've ever reported one of these.  The
difference between copy_from_user() and __copy_from_user() is that the
underscore version means we called
access_ok(addresses_user, sizeof(u64) * q->width) already.  In olden
days if we were doing multiple copies then it was nice to avoid doing
duplicate checks on the fast path.

I don't think we have called access_ok() here.  At one point Linus said
that if we don't call access_ok() in the same function then it should
just automatically considered a bug.  We used to have a lot of issues
with this back in the day...  I don't think anyone took Linus too
seriously on this though.

    165                 if (err) {
    166                         err = -EFAULT;
    167                         goto err_syncs;
    168                 }
    169         }
    170 
    171 retry:
    172         if (!xe_vm_in_lr_mode(vm) && xe_vm_userptr_check_repin(vm)) {
    173                 err = down_write_killable(&vm->lock);
    174                 write_locked = true;
    175         } else {

regards,
dan carpenter