Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [Medium] UAPI extensibility flaw: DMA_BUF_IOCTL_GET_PRIORITY is defined as _IOR and does not validate the 'pad' field on input. --
--- Patch [1]: [RFC PATCH] dma-buf: add a generic reclaim-priority hint --- commit be79050598dd83ece044d5037692f147154efbe0 Author: Ferran Duarri <[email protected]> dma-buf: add a generic reclaim-priority hint This commit introduces a priority hint to dma-buf backing memory to help subsystems decide eviction order under memory pressure. It adds the hint field to struct dma_buf along with GET and SET ioctls to manage it from userspace. > diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c > --- a/drivers/dma-buf/dma-buf.c > +++ b/drivers/dma-buf/dma-buf.c [ ... ] > @@ -580,6 +614,21 @@ static long dma_buf_ioctl(struct file *file, > case DMA_BUF_SET_NAME_B: > return dma_buf_set_name(dmabuf, (const char __user *)arg); > > + case DMA_BUF_IOCTL_SET_PRIORITY: > + if (copy_from_user(&prio, (void __user *)arg, sizeof(prio))) > + return -EFAULT; > + if (prio.pad || prio.priority > DMA_BUF_PRIORITY_MAX) > + return -EINVAL; > + dma_buf_set_priority(dmabuf, prio.priority); > + return 0; > + > + case DMA_BUF_IOCTL_GET_PRIORITY: > + memset(&prio, 0, sizeof(prio)); > + prio.priority = dma_buf_get_priority(dmabuf); > + if (copy_to_user((void __user *)arg, &prio, sizeof(prio))) > + return -EFAULT; > + return 0; [Severity: Medium] Since the pad field in struct dma_buf_priority is documented as "must be zero, reserved for future use", should we validate it in the GET ioctl as well? Because dma_buf_ioctl() does a memset() and copy_to_user() without first calling copy_from_user() to check if the pad is zero, userspace isn't forced to initialize it. If we ever want to use the pad field for input flags or modifiers in the future, we won't be able to do so without breaking existing userspace applications that might be leaving it uninitialized. > diff --git a/include/uapi/linux/dma-buf.h b/include/uapi/linux/dma-buf.h > --- a/include/uapi/linux/dma-buf.h > +++ b/include/uapi/linux/dma-buf.h [ ... ] > @@ -179,5 +210,7 @@ struct dma_buf_import_sync_file { > #define DMA_BUF_SET_NAME_B _IOW(DMA_BUF_BASE, 1, __u64) > #define DMA_BUF_IOCTL_EXPORT_SYNC_FILE _IOWR(DMA_BUF_BASE, 2, struct > dma_buf_export_sync_file) > #define DMA_BUF_IOCTL_IMPORT_SYNC_FILE _IOW(DMA_BUF_BASE, 3, struct > dma_buf_import_sync_file) > +#define DMA_BUF_IOCTL_SET_PRIORITY _IOW(DMA_BUF_BASE, 4, struct > dma_buf_priority) > +#define DMA_BUF_IOCTL_GET_PRIORITY _IOR(DMA_BUF_BASE, 5, struct > dma_buf_priority) [Severity: Medium] If we want to validate the pad field as an input parameter to preserve future extensibility, should this ioctl be defined as _IOWR instead of _IOR? Using _IOR signifies an output-only operation, which dictates the ioctl number and structurally discourages the kernel from reading inputs from the structure. -- Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1
