VK_EXT_aquire_xlib_display and kernel security concerns

[James Jones]

On 10/14/2017 02:00 AM, "Keith Packard" wrote:
> 
> I've implemented this extension in DRM and have run into a conflict
> between the spec and the Linux architecture.
> 
> The VkDisplayKHR parameter for VK_EXT_acquire_xlib_display can be
> found in two different ways:
> 
>   1) Enumerate displays using vkGetPhysicalDeviceDisplayPropertiesKHR
> 
>   2) Enumerate RROutputs using xcb_randr_get_screen_resources and
>      convert that to a VkDisplayKHR with vkGetRandROutputDisplayEXT
> 
> The former makes some assumptions which are not valid in all
> environments:
> 
>   a) It assumes that the Vulkan rendering driver can touch all of the
>      display resources on the system.
> 
>      Under DRM, a regular user running vulkan can only open a 'rendering'
>      node, which denies it all access to the display resources associated
>      with the rendering hardware.
> 
>   b) It assumes that the rendering driver knows about the display
>      hardware.
> 
>      There are many modern systems where the display hardware and
>      rendering hardware are entirely separate, and we must use DMAbufs to
>      get the rendered output sent from the rendering driver to the
>      display driver.
> 
>   c) It assumes that X controls all display resources on the system.
> 
>      If you're running more than one X server on different displays, then
>      you can end up with a VkDisplayKHR object which the X server doesn't
>      manage, and hence cannot be 'acquired'.

I think at a lower level, we have different views of how 
vkGetPhysicalDeviceDisplayPropertiesKHR/VK_KHR_display works. 
vkGetPhysicalDeviceDisplayPropertiesKHR() is suppose to enumerate all 
the displays on a given device.  Vulkan is agnostic to the underlying 
device model, so whether you want to init it on a DRM render or full 
(control? I forget the terminology now.) device is up to the driver.  I 
assumed DRM drivers would try the display-capable node, then fall back 
to a render node if that failed.

Regardless, it is supposed to enumerate all displays, whether you can 
control them and perform modesets on them, acquire them, or none of 
those ops.  Enumeration in itself can be useful for purposes of 
correlation, which would allow apps to discover where their windows are 
without native window system APIs if some future extensions materialize. 
  Everyone focuses on the direct-to-display swapchain portion of 
VK_KHR_display, but the core of it is really just a display enumeration API.

Therefore, for (b), such a device would enumerate no displays.  It would 
be up to the app to do some kind of cross-device interop to get images 
from its render-only device to its display-only device.  Hence all the 
allocator work and stuff.  If the driver/hardware vendor chooses to hide 
that detail when presenting to X/wayland/etc, great, but it's pretty 
hard to hide when doing direct-to-display presentation, as you point 
out.  Vulkan's an explicit API, and I never expected drivers to hide 
this detail for direct-to-display swapchains.

For (c), I hope that after reading the above, there is no such 
assumption.  Of course the app can't acquire such a display, but that 
shouldn't be unexpected.  I had hoped that there would be a way to 
enumerate "hidden" HMD displays using XRandR, and I thought you provided 
that in your XRandR changes.  It seems to be suggested below as well. If 
apps have that, they can use vkGetRandROutputDisplayEXT to map them to 
VkDisplayKHR objects, rather than the other way around.  That way, they 
could avoid an attempt to acquire a display from the wrong X server. 
Going the other way, from Vulkan display enumeration -> X grab, was sort 
of intended to be a stop-gap until RandR caught up.  However, it works 
fine as well if you don't mind a try-and-fail enumeration model.  I just 
try to avoid that when possible.

> I can work around a) by opening up access rights to view display
> resources to all applications with access to render nodes. I've had push
> back on this plan from the DRM maintainers as it dramatically changes
> the access control model used by DRM with unknown consequences for both
> security and application abuse.

FWIW, I think this would be great.  I don't see why enumeration should 
be a privileged operation.  chmod +x the display directory please :-)

> I can't work around b) and c) as the
> vkGetPhysicalDeviceDisplayPropertiesKHR function doesn't have any
> information about the window system to allow it to limit the set of
> display resources to those shared by X and Vulkan.
> 
> Using method 2) provides the necessary control as
> vkGetRandROutputDisplayEXT gets both the Vulkan and X contexts and can
> make suitable choices about what it can support.
> 
> However, the reason vkAcquireXlibDisplay takes a VkDisplayKHR object
> instead of an RROutput parameter is so that it can access displays which
> X may not want to advertise to all clients. I don't think that's
> necessary; we can advertise the RROutput, but just not tell clients
> that it is "Connected" when the display shouldn't be incorporated into a
> 'normal 'desktop. A separate mechanism would be used to report Connected
> status and describe what kind of device is out there.
> 
> So, here's what I propose:
> 
> vkGetPhysicalDeviceDisplayPropertiesKHR will fail when the
> system does not provide access to display resources for unprivileged
> Vulkan applications. When this happens, the application is expected to
> discover the desired display resource through X and map that to a
> VkDisplayKHR using vkGetRandROutputDisplayEXT.
> 
> Any VkDisplayKHR may be used with vkGetDisplayModePropertiesKHR.

Sounds good.

> Once vkAcquireXlibDisplay is called, all of the VK_KHR_display APIs will
> work, but the resources visible will be restricted to the acquired
> display and plane.

Our driver will likely continue to expose all resources, but would fail 
swapchain creation or present if it didn't have access to some resource. 
  I agree there's a bit of an API hole here, but that's true for 
KHR_display in general.  I purposely didn't include a "do I have mode 
set permissions" query in it because I wasn't sure enough what hte 
semantics should be.  It has some hand-wavy language saying swapchain 
creation or present can fail due to lack of permissions instead as a 
place-holder.  As I say though, I prefer not to rely on try-and-fail, so 
perhaps we could add some API here.

> With this secondary mechanism, DRM-based systems will expose all of
> their native functionality without compromising the current API design.

Let me know if you have suggestions to improve the Vulkan APIs.

Thanks,
-James