On Mon, 3 Apr 2023 at 14:00, Emil Velikov <emil.l.velikov@xxxxxxxxx> wrote: > > > I think we should zero num_(in|out)_syncobjs when the respective parse > > > fails. Otherwise we get one "cleanup" within the parse function itself > > > and a second during the cleanup_submit. Haven't looked at it too closely > > > but I suspect that will trigger an UAF or two. > > > > There are checks for NULL pointers in the code that will prevent the > > UAF. I'll add zeroing of the nums for more consistency. > > > > Riiiight the drm_syncobj is attached to the encapsulating struct > virtio_gpu_submit _only_ on success. > By clearing the num variables, the NULL checks will no longer be > needed ... in case you'd want to drop that. > > Either way - even as-is the code is safe. > Err or not. The NULL check itself will cause NULL pointer deref. In more detail: in/out syncobjs are memset() to NULL in virtio_gpu_init_submit(). The virtio_gpu_parse_(|post_)deps() will fail and leave them unchanged. Then virtio_gpu_free_syncobjs() and virtio_gpu_reset_syncobjs() will trigger a NULL ptr deref, because they are accessing the elements of a the (NULL) array. Apart from the num_(in|out)_syncobjcs=0, I would drop the NULL checks - they give a false sense of security IMHO. -Emil _______________________________________________ Virtualization mailing list Virtualization@xxxxxxxxxxxxxxxxxxxxxxxxxx https://lists.linuxfoundation.org/mailman/listinfo/virtualization
