In the commit title: the description should be some sort of action, describing what the commit does. Using "should" doesn't make it clear the commit actually does that.
On 05/05/2023 17:36, Pawel Zalewski wrote:
The module net:sunrpc:cache.c will effectively use the same callback cache_ioctl for pointers passed from file_operations.unlocked_ioctl and proc_ops.proc_ioctl, the pointer passed from file_operations.unlocked_ioctl is of user_uintptr_t type, thus for consistency the argument in .proc_ioctl fp should also be of user_uintptr_t type.
This is not simply a matter of consistency: without using user_uintptr_t, we cannot propagate capabilities in PCuABI. Because at least one proc_ioctl handler (cache_ioctl_procfs) expects the argument to be a pointer, we need to change the callback signature (otherwise we would have been better off not changing it).
Use user_uintptr_t in proc_fs for the argument instead of the unsigned long type.
Signed-off-by: Pawel Zalewski pzalewski@thegoodpenguin.co.uk
include/linux/proc_fs.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/include/linux/proc_fs.h b/include/linux/proc_fs.h index 81d6e4ec2294..90ef95faf16d 100644 --- a/include/linux/proc_fs.h +++ b/include/linux/proc_fs.h @@ -36,9 +36,9 @@ struct proc_ops { loff_t (*proc_lseek)(struct file *, loff_t, int); int (*proc_release)(struct inode *, struct file *); __poll_t (*proc_poll)(struct file *, struct poll_table_struct *);
- long (*proc_ioctl)(struct file *, unsigned int, unsigned long);
- long (*proc_ioctl)(struct file *, unsigned int, user_uintptr_t);
#ifdef CONFIG_COMPAT
- long (*proc_compat_ioctl)(struct file *, unsigned int, unsigned long);
- long (*proc_compat_ioctl)(struct file *, unsigned int, user_uintptr_t);
We have avoided doing that for other compat ioctl callbacks, including the main file_operations one, because it is semantically incorrect: in compat(64), the argument really is just an unsigned long.
Instead of changing this callback signature, it would be better to fix the existing code that sets a proc_compat_ioctl callback. AFAICT we only need to fix proc_bus_pci_ops, and we can do it in the same way as esas2r_proc_ops, i.e. by using the compat_*_ioctl helpers. proc_bus_pci_ioctl() does not expect a pointer, so we should use the compat_noptr_ioctl helper there. See [1] for more information.
Kevin
[1] https://git.morello-project.org/morello/kernel/linux/-/blob/morello/next/Doc...
#endif int (*proc_mmap)(struct file *, struct vm_area_struct *); unsigned long (*proc_get_unmapped_area)(struct file *, unsigned long, unsigned long, unsigned long, unsigned long);