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

Carsten Haitzler <carsten.haitzler@xxxxxxxxxxxx> · Fri, 14 Jun 2024 11:39:01 +0100

On 6/11/24 5:17 PM, T.J. Mercier wrote:
On Tue, Jun 11, 2024 at 2:35 AM <carsten.haitzler@xxxxxxxxxxxx> wrote:

From: Carsten Haitzler <carsten.haitzler@xxxxxxxxxxxx>

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))))

Aaaah yes. I'm living in the past as usual. :)

can be simply reduced to:
_Alignas(u64)

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

Yup. Will do. Expect a V2 to come in.


Signed-off-by: Carsten Haitzler <carsten.haitzler@xxxxxxxxxxxx>
---
  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







