VK_EXT_aquire_xlib_display and kernel security concerns

[James Jones]

On 10/16/2017 12:28 PM, Keith Packard wrote:
> James Jones <jajones at nvidia.com> writes:
> 
>> 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.
> 
> In many devices, the display controller and rendering hardware are
> separate devices, so it's up to user space to figure out the connection
> between them, and user DMAbufs to pass rendered output to the display.

Understood.  In that case, no displays should be enumerated on the 
"rendering hardware" device in Vulkan, unless the driver vendor opts for 
heroics.

>> I assumed DRM drivers would try the display-capable node, then fall
>> back to a render node if that failed.
> 
> No, they cannot as the display node is protected from normal user
> access.

Right, but normal processes don't need the display node, or aren't 
supposed to.  I assumed apps that need direct-to-display would have 
elevated permissions on systems configured to require it, or use 
"acquire" extensions, potentially combined with native secondary auth 
mechanisms, to get access.  This is why it bothers me that "normal user 
access" doesn't include display enumeration.

>>    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.
> 
> Right, I think all I really need is to hand the driver a connection to
> the X server and it can use that to enumerate the available displays
> through the Vulkan API.
> 
>> 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.
> 
> Yes, we're figuring out how to do that now. Dave has posted proposed
> kernel patches to the list which adds an EDID-based quirk to mark HMD
> displays as 'special'.
> 
> Our current thought is that we would list the 'special' outputs as
> Disconnected, but provide an EDID property for them. That way, normal X
> applications would continue to ignore them, but a special application
> could still discover the target device using EDID information.

Cool.

>> 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.
> 
> Yes, I have a test application which works this way. However, as the
> nVidia driver is hiding HMD outputs from X applications, an application
> wanting to support both nVidia and DRM drivers will have to try both
> methods.

Once a method is finalized to expose HMDs without having desktops pick 
them up, we can probably switch to that behavior.  We'll have to review 
the proposed disconnected-but-has-EDID idea above.  Currently, we have 
an option to force exposure of HMDs, but then they just look like normal 
displays to userspace.

>> FWIW, I think this would be great.  I don't see why enumeration should
>> be a privileged operation.  chmod +x the display directory please :-)
> 
> That was my initial plan, and it obviously works fine. Daniel Vetter
> has a reasonable aversion to providing this kind of access; "bad things"
> have happened in the past when we expose system information of this kind
> to all applications.
> 
> Besides, this fails in the case where the display device and rendering
> device are separate, so we end up needing another path for that anyways.

Given the current definition of a Vulkan-capable device, yes.  It's 
annoying that Vulkan can't yet expose a device that only has displays 
and the ability to import memory objects and create images from them.

I prefer the enumeration ability as a general principle though.  Is the 
discussion with Daniel Vetter captured somewhere so I can read up on the 
objections?  I couldn't find it with a naive Google or DRI devel search.

>> Let me know if you have suggestions to improve the Vulkan APIs.
> 
> Would it be reasonable to add a new function to the acquire_xlib_display
> extension that would let you pass the Display into the API before the
> vkGetPhysicalDeviceDisplayPropertiesKHR call? That would offer the least
> impact on applications using the API, allowing them to have a single
> enumeration mechanism for both DRM and nVidia devices. Right now, the
> pattern looks like:
> 
>          open X connection
>          enumerate displays using vk APIs
>          select desired display
>          acquire xlib display
> 
> That would change to
> 
>          open X connection
>          tell vk about the X connection
>          enumerate displays using vk APIs
>          select desired display
>          acquire xlib display

We've very much tried to avoid giving Vulkan itself stateful knowledge 
of a window system.  Swapchains have it, and that's it, which I feel is 
a rather clean design (others prefer swapchains not exist, which would 
be even cleaner, but... compromises).  EXT_acquire_xlib_display kind of 
has it, but it's tied to a specific object much like the swapchain case. 
  From a design perspective, I don't like the idea that a top-level 
Vulkan object, especially a physical device which is supposed to be 
immutable, would take on new properties because of which window system 
it's associated with.  However, if there are one or two commands that 
could benefit from a display connection, an alternative design would be 
to make an extension that provides the connection info to those commands 
and scope it's availability to Vulkan to be the duration of those commands.

> Without this function, every application would have to try both
> enumerations:
> 
>          open X connection
>          enumerate displays using vk APIS
>          if (success) {
>                  select desired display
>          } else {
>                  enumerate outputs using X apis
>                  select desired output
>                  map output to display
>          }
>          acquire xlib display
> 
> Alternatively, we could encourage applications to always use X lib APIs
> for selecting the output; there's more information provided about the
> device from that interface (like the EDID). That would require that the
> nVidia X driver not hide the HMD from X as it reportedly does today.

This is my preferred direction.

> Oh, one added benefit of telling Vulkan about the X display is that it
> could trim the list of outputs to those reachable by the rendering
> driver; definitely a point in favor of that method. In this case, we
> might want to extend the information made available through Vulkan to
> things like EDID so that applications aren't left guessing which output
> they want based solely on the display geometry.

There was more information in the original KHR_display extension draft, 
but people claimed it was overly verbose.  Some people felt raw EDID 
itself was too low-level I think.  We could revisit that though.  Note I 
still don't think it's a good idea to enumerate outputs not reachable by 
the rendering device anyway, so I don't see this as a positive for 
telling Vulkan more about X.

Thanks,
-James