Re: [PATCH 3/5] drm/vmwgfx: use core drm to extend/check vmw_execbuf_ioctl

[Emil Velikov <emil.l.velikov@gmail.com>]

On 2019/05/24, Daniel Vetter wrote:
> On Fri, May 24, 2019 at 8:05 AM Thomas Hellstrom <thellstrom@vmware.com> 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@vmware.com> wrote:
> > > > Hi, Emil,
> > > >
> > > > On Wed, 2019-05-22 at 17:41 +0100, Emil Velikov wrote:
> > > > > From: Emil Velikov <emil.velikov@collabora.com>
> > > > >
> > > > > 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?
Emil
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel