gfx: Struct traits for GpuMemoryBufferHandle.

ui/gfx/mojo/buffer_types.mojom

 // Copyright 2016 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 module gfx.mojom;
 
 // gfx::BufferFormat
 enum BufferFormat {
   ATC,
   ATCIA,
@@ skipping 31 matching lines @@
   IO_SURFACE_BUFFER,
   SURFACE_TEXTURE_BUFFER,
   OZONE_NATIVE_PIXMAP,
   LAST = OZONE_NATIVE_PIXMAP
 };
 
 // gfx::GpuMemoryBufferId
 struct GpuMemoryBufferId {
   int32 id;
 };
+
+// gfx::NativePixmapPlane
+struct NativePixmapPlane {
+  int32 stride;

[Tom Sepez, 2016/08/31 16:52:36]

Can these be negative?  Would an unsigned type be more appropriate? Otherwise we
have to check for < 0 in a lot of places, no?

[spang, 2016/08/31 19:35:57]

Please don't do that.

https://engdoc.corp.google.com/eng/doc/devguide/cpp/styleguide.shtml?cl=head#...

[sadrul, 2016/08/31 19:53:19]

Doing this separately in https://codereview.chromium.org/2298353002/.

[rjkroege, 2016/08/31 19:54:14]

Also, I recall that for some overlay plane types, the stride might need to be
negative to make some planes (e.g. overlay planes scan out with the opposite
coordinate system from the GPU.)

+  int32 offset;
+  uint64 modifier;
+};
+
+// gfx::NativePixmapHandle
+struct NativePixmapHandle {
+  // A file descriptor for the underlying memory object (usually dmabuf).
+  array<handle> fds;
+
+  array<NativePixmapPlane> planes;

[Tom Sepez, 2016/08/31 17:04:15]

Anyways, where do we check upon de-serialization that the combination of strides
and offsets etc. doesn't overflow the bounds of the underlying memory?

[sadrul, 2016/08/31 17:13:19]

It doesn't look like we actually check that anywhere currently. The host (i.e.
chrome browser, or the window server) receives these from the gpu process, and
sends them off back to the clients (i.e. renderers) or processes them locally,
without doing any bounds checking.

[rjkroege, 2016/08/31 19:54:14]

On CrOS, these values end up here:
https://cs.chromium.org/chromium/src/ui/ozone/platform/drm/common/client_nati...


trying to think like a security reviewer for a moment: if !pwned client invokes
the client pixmap code with bad parameters from pwned gpu process (pixmap
handles always originate in gpu process) then there would seem to be two cases?
a) mmap is fragile and badness ensures or b) mmap is robust and client has
either done nothing (and handles the mmap error) or added a mapping for a
possibly over large area of memory that it will write raster image into.

Seems like the deserializer should test that size.height_* stride is > 0 and
complain bitterly if isn't.

[sadrul, 2016/08/31 20:01:28]

|size| doesn't come at the same time as the stride though, so perhaps we can't
do the validation during deserialization. Should we do more validation at the
place where we use these values (i.e. in
https://cs.chromium.org/chromium/src/ui/ozone/platform/drm/common/client_nati...)
instead?

+};
+
+// gfx::GpuMemoryBufferHandle
+struct GpuMemoryBufferHandle {
+  GpuMemoryBufferType type;
+  GpuMemoryBufferId id;
+  handle shared_memory_handle;
+  uint32 offset;
+  int32 stride;

[Tom Sepez, 2016/08/31 16:52:36]

nit: same here.

+  NativePixmapHandle? native_pixmap_handle;
+  // TODO: Add support for mach_port on mac.
+};