VFIO_IOMMU_GET_INFO capability struct alignment

VFIO_IOMMU_GET_INFO capability struct alignment

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1322 bytes --]

Hi,
It appears that ioctl(VFIO_IOMMU_GET_INFO) can produce misaligned
capability structures. Userspace workarounds exist but I wanted to ask
whether the kernel can align capability structures to save all userspace
programs the trouble?

The issue is:

  struct vfio_iommu_type1_info_dma_avail {
          struct vfio_info_cap_header header;              /*     0     8 */
          __u32                      avail;                /*     8     4 */
  
          /* size: 12, cachelines: 1, members: 2 */
          /* last cacheline: 12 bytes */
  };

Once this capability is added, the next capability will be 4-byte
aligned but not 8-byte aligned. If there are __u64 fields in the next
capability, then they will be misaligned.

This was noticed when investigating a bug in userspace code that uses
ioctl(VFIO_IOMMU_GET_INFO):
https://gitlab.com/pci-driver/pci-driver/-/merge_requests/2#note_1495734084

One possible solution is to modify vfio_info_cap_add() so that
capability structures are always rounded up to 8 bytes. This does not
break the uapi because capability structure offsets are described at
runtime via the cap_offset and header->next fields. Existing userspace
programs would continue to work and all programs would find that
capability structures are now aligned.

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: VFIO_IOMMU_GET_INFO capability struct alignment

[Alex Williamson]

On Tue, 1 Aug 2023 11:38:46 -0400
Stefan Hajnoczi <stefanha@redhat.com> wrote:

> Hi,
> It appears that ioctl(VFIO_IOMMU_GET_INFO) can produce misaligned
> capability structures. Userspace workarounds exist but I wanted to ask
> whether the kernel can align capability structures to save all userspace
> programs the trouble?
> 
> The issue is:
> 
>   struct vfio_iommu_type1_info_dma_avail {
>           struct vfio_info_cap_header header;              /*     0     8 */
>           __u32                      avail;                /*     8     4 */
>   
>           /* size: 12, cachelines: 1, members: 2 */
>           /* last cacheline: 12 bytes */
>   };
> 
> Once this capability is added, the next capability will be 4-byte
> aligned but not 8-byte aligned. If there are __u64 fields in the next
> capability, then they will be misaligned.
> 
> This was noticed when investigating a bug in userspace code that uses
> ioctl(VFIO_IOMMU_GET_INFO):
> https://gitlab.com/pci-driver/pci-driver/-/merge_requests/2#note_1495734084
> 
> One possible solution is to modify vfio_info_cap_add() so that
> capability structures are always rounded up to 8 bytes. This does not
> break the uapi because capability structure offsets are described at
> runtime via the cap_offset and header->next fields. Existing userspace
> programs would continue to work and all programs would find that
> capability structures are now aligned.

Yes, I think the helpers should automatically align each added
capability.  Thanks,

Alex

Re: VFIO_IOMMU_GET_INFO capability struct alignment

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1741 bytes --]

On Tue, Aug 01, 2023 at 10:17:30AM -0600, Alex Williamson wrote:
> On Tue, 1 Aug 2023 11:38:46 -0400
> Stefan Hajnoczi <stefanha@redhat.com> wrote:
> 
> > Hi,
> > It appears that ioctl(VFIO_IOMMU_GET_INFO) can produce misaligned
> > capability structures. Userspace workarounds exist but I wanted to ask
> > whether the kernel can align capability structures to save all userspace
> > programs the trouble?
> > 
> > The issue is:
> > 
> >   struct vfio_iommu_type1_info_dma_avail {
> >           struct vfio_info_cap_header header;              /*     0     8 */
> >           __u32                      avail;                /*     8     4 */
> >   
> >           /* size: 12, cachelines: 1, members: 2 */
> >           /* last cacheline: 12 bytes */
> >   };
> > 
> > Once this capability is added, the next capability will be 4-byte
> > aligned but not 8-byte aligned. If there are __u64 fields in the next
> > capability, then they will be misaligned.
> > 
> > This was noticed when investigating a bug in userspace code that uses
> > ioctl(VFIO_IOMMU_GET_INFO):
> > https://gitlab.com/pci-driver/pci-driver/-/merge_requests/2#note_1495734084
> > 
> > One possible solution is to modify vfio_info_cap_add() so that
> > capability structures are always rounded up to 8 bytes. This does not
> > break the uapi because capability structure offsets are described at
> > runtime via the cap_offset and header->next fields. Existing userspace
> > programs would continue to work and all programs would find that
> > capability structures are now aligned.
> 
> Yes, I think the helpers should automatically align each added
> capability.  Thanks,

Thanks, I will give it a try and post a patch.

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]