[PATCH] drm: Fix alignment of temporary stack ioctl buffers

[T.J. Mercier]

On Tue, Jun 11, 2024 at 2:35 AM <carsten.haitzler at foss.arm.com> wrote:
>
> From: Carsten Haitzler <carsten.haitzler at foss.arm.com>
>
> In a few places (core drm + AMD kfd driver), the ioctl handling uses a
> temporary 128 byte buffer on the stack to copy to/from user. ioctl data
> can have structs with types of much larger sizes than a byte and a
> system may require alignment of types in these. At the same time the
> compiler may align a char buf to something else as it has no idea that
> this buffer is used for storing structs with such alignment
> requirements. At a minimum putting in alignment information as an
> attribute should be a warning in future if an architecture that needs
> more alignment appears.
>
> This was discovered while implementing capability ABI support in Linux
> on ARM's Morello CPU (128 bit capability "pointers" in userspace, with
> a 64bit non-capability kernel (hybrid) setup). In this, userspace
> ioctl structs now had to transport capabilities that needed 16 byte
> alignment, but the kernel was not putting these data buffers on that
> alignment boundary.
>
> Currently the largest type that is needed is a u64 so the alignment
> only asks for that.

Makes sense to me.

Now that the kernel depends on C11, I think:
__attribute__((aligned(__alignof__(u64))))

can be simply reduced to:
_Alignas(u64)

and put first instead of last in the declaration:
_Alignas(u64) char stack_kdata[128];

>
> Signed-off-by: Carsten Haitzler <carsten.haitzler at foss.arm.com>
> ---
>  drivers/dma-buf/dma-heap.c               | 2 +-
>  drivers/gpu/drm/amd/amdkfd/kfd_chardev.c | 2 +-
>  drivers/gpu/drm/drm_ioctl.c              | 2 +-
>  3 files changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/dma-buf/dma-heap.c b/drivers/dma-buf/dma-heap.c
> index 84ae708fafe7..a49e20440edf 100644
> --- a/drivers/dma-buf/dma-heap.c
> +++ b/drivers/dma-buf/dma-heap.c
> @@ -126,7 +126,7 @@ static unsigned int dma_heap_ioctl_cmds[] = {
>  static long dma_heap_ioctl(struct file *file, unsigned int ucmd,
>                            unsigned long arg)
>  {
> -       char stack_kdata[128];
> +       char stack_kdata[128] __attribute__((aligned(__alignof__(u64))));
>         char *kdata = stack_kdata;
>         unsigned int kcmd;
>         unsigned int in_size, out_size, drv_size, ksize;
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c b/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
> index fdf171ad4a3c..69a0aae0f016 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_chardev.c
> @@ -3236,7 +3236,7 @@ static long kfd_ioctl(struct file *filep, unsigned int cmd, unsigned long arg)
>         amdkfd_ioctl_t *func;
>         const struct amdkfd_ioctl_desc *ioctl = NULL;
>         unsigned int nr = _IOC_NR(cmd);
> -       char stack_kdata[128];
> +       char stack_kdata[128] __attribute__((aligned(__alignof__(u64))));
>         char *kdata = NULL;
>         unsigned int usize, asize;
>         int retcode = -EINVAL;
> diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
> index e368fc084c77..aac3d5a539a6 100644
> --- a/drivers/gpu/drm/drm_ioctl.c
> +++ b/drivers/gpu/drm/drm_ioctl.c
> @@ -767,7 +767,7 @@ long drm_ioctl(struct file *filp,
>         drm_ioctl_t *func;
>         unsigned int nr = DRM_IOCTL_NR(cmd);
>         int retcode = -EINVAL;
> -       char stack_kdata[128];
> +       char stack_kdata[128] __attribute__((aligned(__alignof__(u64))));
>         char *kdata = NULL;
>         unsigned int in_size, out_size, drv_size, ksize;
>         bool is_driver_ioctl;
> --
> 2.25.1
>