On Fri, Feb 8, 2019 at 2:06 PM Greg KH <[email protected]> wrote: > > On Mon, Feb 04, 2019 at 10:32:44PM +0200, Oded Gabbay wrote: > > +int hl_cb_ioctl(struct hl_fpriv *hpriv, void *data) > > +{ > > + union hl_cb_args *args = data; > > + struct hl_device *hdev = hpriv->hdev; > > + u64 handle; > > + int rc; > > + > > + switch (args->in.op) { > > + case HL_CB_OP_CREATE: > > + rc = hl_cb_create(hdev, &hpriv->cb_mgr, args->in.cb_size, > > + &handle, hpriv->ctx->asid); > > so cb_size comes from userspace, ok, you check for the value to be too > small, but not too big. That means someone can try to allocate too much > memory, possibly crashing things, not good :( Yes, correct, but even if I limit a single allocation to, let's say, 1MB, what's stopping a userspace process from allocating multiple CBs and draining the system memory ? I'm counting on the oom module to kill that process if it mis-behaves. And, btw, I assumed there is hard limit of 4MB on a single dma_alloc_coherent. i.e. I was never able to allocate more then 4MB through that API. So I never thought I need to check for max size because of that hard limit. Am I missing something here ?
> > > > + memset(args, 0, sizeof(*args)); > > + args->out.cb_handle = handle; > > + break; > > + case HL_CB_OP_DESTROY: > > + rc = hl_cb_destroy(hdev, &hpriv->cb_mgr, > > + args->in.cb_handle); > > + memset(args, 0, sizeof(*args)); > > Why zero this if it's not copied back to userspace? Fixed > > > > > + break; > > + default: > > + rc = -EINVAL; > > -ENOTTY is normally the "invalid ioctl value", right? Fixed > > > + break; > > + } > > + > > + return rc; > > +} > > thanks, > > greg k-h
