On 5/24/19 12:53 PM, Emil Velikov wrote:
On 2019/05/24, Daniel Vetter wrote:
On Fri, May 24, 2019 at 8:05 AM Thomas Hellstrom <thellstrom@xxxxxxxxxx> wrote:
On Wed, 2019-05-22 at 21:09 +0200, Daniel Vetter wrote:
On Wed, May 22, 2019 at 9:01 PM Thomas Hellstrom <
thellstrom@xxxxxxxxxx> wrote:
Hi, Emil,
On Wed, 2019-05-22 at 17:41 +0100, Emil Velikov wrote:
From: Emil Velikov <emil.velikov@xxxxxxxxxxxxx>
Currently vmw_execbuf_ioctl() open-codes the permission checking,
size
extending and copying that is already done in core drm.
Kill all the duplication, adding a few comments for clarity.
Ah, there is core functionality for this now.
What worries me though with the core approach is that the sizes are
not
capped by the size of the kernel argument definition, which makes
mailicious user-space being able to force kmallocs() the size of
the
maximum ioctl size. Should probably be fixed before pushing this.
Hm I always worked under the assumption that kmalloc and friends
should be userspace hardened. Otherwise stuff like kmalloc_array
doesn't make any sense, everyone just feeds it unchecked input and
expects that helper to handle overflows.
If we assume kmalloc isn't hardened against that, then we have a much
bigger problem than just vmwgfx ioctls ...
After checking the drm_ioctl code I realize that what I thought was new
behaviour actually has been around for a couple of years, so
fixing isn't really tied to this patch series...
What caused me to react was that previously we used to have this
e4fda9f264e1 ("drm: Perform ioctl command validation on the stored
kernel values")
and we seem to have lost that now, if not for the io flags then at
least for the size part. For the size of the ioctl arguments, I think
in general if the kernel only touches a subset of the user-space
specified size I see no reason why we should malloc / copy more than
that?
I guess we could optimize that, but we'd probably still need to zero
clear the added size for forward compat with newer userspace. Iirc
we've had some issues in this area.
Now, given the fact that the maximum ioctl argument size is quite
limited, that might not be a big problem or a problem at all. Otherwise
it would be pretty easy for a malicious process to allocate most or all
of a system's resident memory?
The biggest you can allocate from kmalloc is limited by the largest
contiguous chunk alloc_pages gives you, which is limited by MAX_ORDER
from the page buddy allocator. You need lots of process to be able to
exhaust memory like that (and like I said, the entire kernel would be
broken if we'd consider this a security issue). If you want to make
sure that a process group can't exhaust memory this way then you need
to set appropriate cgroups limits.
I do agree with all the sentiments that drm_ioctl() could use some extra
optimisation and hardening. At the same time I would remind that the
code has been used as-is by vmwgfx and other drivers for years.
In other words: let's keep that work as orthogonal series.
What do you guys think?
I agree. Then I only had a concern with one of the patches.
/Thomas
Emil
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
