Re: [PATCH 4/5] drm/vmwgfx: remove custom ioctl io encoding check

From: Emil Velikov <emil.l.velikov@gmail.com>
To: Thomas Hellstrom <thellstrom@vmware.com>
Cc: "kernel@collabora.com" <kernel@collabora.com>,
	Linux-graphics-maintainer <Linux-graphics-maintainer@vmware.com>,
	"dri-devel@lists.freedesktop.org"
	<dri-devel@lists.freedesktop.org>
Subject: Re: [PATCH 4/5] drm/vmwgfx: remove custom ioctl io encoding check
Date: Mon, 27 May 2019 13:35:52 +0100	[thread overview]
Message-ID: <20190527123552.GD15067@arch-x1c3> (raw)
In-Reply-To: <c12af67b4b730a0f8b7d61a349e8e5845b14be1d.camel@vmware.com>

Hi Thomas,

On 2019/05/27, Thomas Hellstrom wrote:

> > I think we might be talking past each other, let's take a step back:
> > 
> >  - as of previous patch, all of vmwgfx ioctls size is consistently
> > handled by the core
> 
> I don't think I follow you here, AFAICT patch 3/5 only affects and
> relaxes the execbuf checking (and in fact a little more than I would
> like)?
> 
Precisely, it makes execbuf ioctl behave like all other ioctls - both
vmwgfx and rest of drm.

> >  - handling of featue flags, as always, is responsibility of the
> > driver
> > ifself
> >  - with this patch, ioctl direction is also handled by core.
> > 
> > Here core ensures we only copy in/out as much data as the kernel
> > implementation can handle.
> > 
> > 
> > Let's consider the following real world example - msm and virtio_gpu.
> > 
> > An in field of an _IOW ioctl becomes in/out aka _IORW ioctl.
> >  - we add a flag to annotate/request the out, as always invalid flags
> > are return -EINVAL
> >  - we change the ioctl encoding
> > 
> > As currently handled by core DRM, old kernel/new userspace and
> > vice-versa works just fine. Sadly, vmwgfx will error out, while it
> > could
> > be avoided.
> 
> IMO basically we have a tradeoff between strict checking in this case,
> and new user-space vs old kernel "hazzle-free" transition in the
> relaxed case. 
> 
Precisely. If I read the code correctly, ATM new userspace will fail
against old kernels. Unless userspace writes two versions of the ioctl -
with with each encoding.

> > 
> > As said above, I'll gladly adjust core and/or others, if this relaxed
> > approach causes an issue somewhere. A specific use-case, real or
> > hypothetical will be appreciated.
> 
> To me there are two important reasons to keep the strict approach.
> 
> 1) Avoid user-space mistakes early in the development cycle. We can't
> distinguish between buggy user-space and "new" user-space. This is
> important because of [a]) below.
> 
Can you provide a concrete example, please?

> 2) Catch a lot of fuzzer combinations and error out early instead of
> forwarding them to the ioctl function where they may cause harm.
> 
Struggling to see why this is a problem? At some point the fuzzer will
get past this first line of defence, so we want to make the rest of the
ioctl is robust.

> I think the new user-space vs old kernel can be handled nicely in user-
> space with feature flags or API versions. That's the way we've handled
> them up to now?
> 
How is a feature flag doing to help if the encoding changes from _IOW
to _IORW?

Thanks
Emil
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel