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
