Hi all,
This patch series addresses the issue reported in the following issue: https://git.morello-project.org/morello/kernel/linux/-/issues/45
Changes available at: https://git.morello-project.org/Sevenarth/linux/-/commits/morello/6.1-ioctl-...
v2: - discarded the posix_clock_compat_ioctl handler and replaced with the compat_ptr_ioctl helper function
Luca Vizzarro (7): ptp: Modify ptp_ioctl to use user_uintptr_t tty: Modify ioctl to use user_uintptr_t fs: Fix user_uinptr_t behaviour with ioctl net: Modify ioctl to use user_uintptr_t net/br_ioctl: Modify br_dev_read_uargs to accept user_uintptr_t net/br_ioctl: Support 64-bit pointers in compat time/posix_clock: Use compat_ptr_ioctl helper
drivers/ptp/ptp_chardev.c | 2 +- drivers/ptp/ptp_private.h | 2 +- drivers/tty/n_tty.c | 2 +- drivers/tty/pty.c | 4 ++-- drivers/tty/serial/serial_core.c | 2 +- drivers/tty/tty_ioctl.c | 2 +- drivers/tty/vt/keyboard.c | 2 +- drivers/tty/vt/vt.c | 2 +- drivers/tty/vt/vt_ioctl.c | 8 ++++---- fs/autofs/root.c | 2 +- fs/ext4/ioctl.c | 4 ++-- include/linux/net.h | 2 +- include/linux/posix-clock.h | 2 +- include/linux/tty.h | 4 ++-- include/linux/tty_driver.h | 2 +- include/linux/tty_ldisc.h | 2 +- include/linux/vt_kern.h | 4 ++-- include/net/inet_common.h | 2 +- include/net/ipv6.h | 2 +- include/net/sock.h | 2 +- include/net/tcp.h | 2 +- include/net/udp.h | 2 +- kernel/time/posix-clock.c | 21 +-------------------- net/bridge/br_ioctl.c | 12 ++++++------ net/ipv4/af_inet.c | 2 +- net/ipv4/raw.c | 2 +- net/ipv4/tcp.c | 2 +- net/ipv4/udp.c | 2 +- net/ipv6/af_inet6.c | 2 +- net/ipv6/raw.c | 2 +- net/netlink/af_netlink.c | 2 +- net/packet/af_packet.c | 2 +- net/socket.c | 2 +- net/unix/af_unix.c | 4 ++-- 34 files changed, 47 insertions(+), 66 deletions(-)
The third argument of ioctl handlers is a user input which can contain either a pointer to data in the user space or any number. This commit changes the type of the argument from unsigned long to user_uintptr_t so that we can accommodate the bigger size of capabilities and accept them from user space correctly without discarding their metadata.
Signed-off-by: Luca Vizzarro Luca.Vizzarro@arm.com --- drivers/ptp/ptp_chardev.c | 2 +- drivers/ptp/ptp_private.h | 2 +- include/linux/posix-clock.h | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/ptp/ptp_chardev.c b/drivers/ptp/ptp_chardev.c index af3bc65c4595..397e4ba71d90 100644 --- a/drivers/ptp/ptp_chardev.c +++ b/drivers/ptp/ptp_chardev.c @@ -106,7 +106,7 @@ int ptp_open(struct posix_clock *pc, fmode_t fmode) return 0; }
-long ptp_ioctl(struct posix_clock *pc, unsigned int cmd, unsigned long arg) +long ptp_ioctl(struct posix_clock *pc, unsigned int cmd, user_uintptr_t arg) { struct ptp_clock *ptp = container_of(pc, struct ptp_clock, clock); struct ptp_sys_offset_extended *extoff = NULL; diff --git a/drivers/ptp/ptp_private.h b/drivers/ptp/ptp_private.h index 77918a2c6701..3bd83bc6e209 100644 --- a/drivers/ptp/ptp_private.h +++ b/drivers/ptp/ptp_private.h @@ -118,7 +118,7 @@ int ptp_set_pinfunc(struct ptp_clock *ptp, unsigned int pin, enum ptp_pin_function func, unsigned int chan);
long ptp_ioctl(struct posix_clock *pc, - unsigned int cmd, unsigned long arg); + unsigned int cmd, user_uintptr_t arg);
int ptp_open(struct posix_clock *pc, fmode_t fmode);
diff --git a/include/linux/posix-clock.h b/include/linux/posix-clock.h index 468328b1e1dd..184aaa05d0ff 100644 --- a/include/linux/posix-clock.h +++ b/include/linux/posix-clock.h @@ -51,7 +51,7 @@ struct posix_clock_operations { * Optional character device methods: */ long (*ioctl) (struct posix_clock *pc, - unsigned int cmd, unsigned long arg); + unsigned int cmd, user_uintptr_t arg);
int (*open) (struct posix_clock *pc, fmode_t f_mode);
The third argument of ioctl handlers is a user input which can contain either a pointer to data in the user space or any number. This commit changes the type of the argument from unsigned long to user_uintptr_t so that we can accommodate the bigger size of capabilities and accept them from user space correctly without discarding their metadata. It also updates the signature of child functions and casts the argument to unsigned long when it must be treated as such for size reasons.
Signed-off-by: Luca Vizzarro Luca.Vizzarro@arm.com --- drivers/tty/n_tty.c | 2 +- drivers/tty/pty.c | 4 ++-- drivers/tty/serial/serial_core.c | 2 +- drivers/tty/tty_ioctl.c | 2 +- drivers/tty/vt/keyboard.c | 2 +- drivers/tty/vt/vt.c | 2 +- drivers/tty/vt/vt_ioctl.c | 8 ++++---- include/linux/tty.h | 4 ++-- include/linux/tty_driver.h | 2 +- include/linux/tty_ldisc.h | 2 +- include/linux/vt_kern.h | 4 ++-- 11 files changed, 17 insertions(+), 17 deletions(-)
diff --git a/drivers/tty/n_tty.c b/drivers/tty/n_tty.c index 597019690ae6..fd58d19c1dc4 100644 --- a/drivers/tty/n_tty.c +++ b/drivers/tty/n_tty.c @@ -2463,7 +2463,7 @@ static unsigned long inq_canon(struct n_tty_data *ldata) }
static int n_tty_ioctl(struct tty_struct *tty, unsigned int cmd, - unsigned long arg) + user_uintptr_t arg) { struct n_tty_data *ldata = tty->disc_data; int retval; diff --git a/drivers/tty/pty.c b/drivers/tty/pty.c index aa6c5fa3d562..255f91b0d515 100644 --- a/drivers/tty/pty.c +++ b/drivers/tty/pty.c @@ -451,7 +451,7 @@ static void pty_remove(struct tty_driver *driver, struct tty_struct *tty) }
static int pty_bsd_ioctl(struct tty_struct *tty, - unsigned int cmd, unsigned long arg) + unsigned int cmd, user_uintptr_t arg) { switch (cmd) { case TIOCSPTLCK: /* Set PT Lock (disallow slave open) */ @@ -640,7 +640,7 @@ int ptm_open_peer(struct file *master, struct tty_struct *tty, int flags) }
static int pty_unix98_ioctl(struct tty_struct *tty, - unsigned int cmd, unsigned long arg) + unsigned int cmd, user_uintptr_t arg) { switch (cmd) { case TIOCSPTLCK: /* Set PT Lock (disallow slave open) */ diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c index 6b86149cd879..122c768893fd 100644 --- a/drivers/tty/serial/serial_core.c +++ b/drivers/tty/serial/serial_core.c @@ -1511,7 +1511,7 @@ static int uart_set_iso7816_config(struct uart_port *port, * Called via sys_ioctl. We can use spin_lock_irq() here. */ static int -uart_ioctl(struct tty_struct *tty, unsigned int cmd, unsigned long arg) +uart_ioctl(struct tty_struct *tty, unsigned int cmd, user_uintptr_t arg) { struct uart_state *state = tty->driver_data; struct tty_port *port = &state->port; diff --git a/drivers/tty/tty_ioctl.c b/drivers/tty/tty_ioctl.c index ce511557b98b..45a5538cdc4a 100644 --- a/drivers/tty/tty_ioctl.c +++ b/drivers/tty/tty_ioctl.c @@ -760,7 +760,7 @@ static int tty_change_softcar(struct tty_struct *tty, int arg) * consistent mode setting. */
-int tty_mode_ioctl(struct tty_struct *tty, unsigned int cmd, unsigned long arg) +int tty_mode_ioctl(struct tty_struct *tty, unsigned int cmd, user_uintptr_t arg) { struct tty_struct *real_tty; void __user *p = (void __user *)arg; diff --git a/drivers/tty/vt/keyboard.c b/drivers/tty/vt/keyboard.c index be8313cdbac3..e001fc67151c 100644 --- a/drivers/tty/vt/keyboard.c +++ b/drivers/tty/vt/keyboard.c @@ -2109,7 +2109,7 @@ int vt_do_kdgkb_ioctl(int cmd, struct kbsentry __user *user_kdgkb, int perm) return ret; }
-int vt_do_kdskled(unsigned int console, int cmd, unsigned long arg, int perm) +int vt_do_kdskled(unsigned int console, int cmd, user_uintptr_t arg, int perm) { struct kbd_struct *kb = &kbd_table[console]; unsigned long flags; diff --git a/drivers/tty/vt/vt.c b/drivers/tty/vt/vt.c index 981d2bfcf9a5..8f7bc37fab54 100644 --- a/drivers/tty/vt/vt.c +++ b/drivers/tty/vt/vt.c @@ -3181,7 +3181,7 @@ static struct console vt_console_driver = { * set_selection_user has locking, and definitely needs it */
-int tioclinux(struct tty_struct *tty, unsigned long arg) +int tioclinux(struct tty_struct *tty, user_uintptr_t arg) { char type, data; char __user *p = (char __user *)arg; diff --git a/drivers/tty/vt/vt_ioctl.c b/drivers/tty/vt/vt_ioctl.c index e120bd655b0e..1106077f1b0c 100644 --- a/drivers/tty/vt/vt_ioctl.c +++ b/drivers/tty/vt/vt_ioctl.c @@ -281,7 +281,7 @@ static int vt_kdsetmode(struct vc_data *vc, unsigned long mode) }
static int vt_k_ioctl(struct tty_struct *tty, unsigned int cmd, - unsigned long arg, bool perm) + user_uintptr_t arg, bool perm) { struct vc_data *vc = tty->driver_data; void __user *up = (void __user *)arg; @@ -734,7 +734,7 @@ static int vt_resizex(struct vc_data *vc, struct vt_consize __user *cs) * capability to modify any console, not just the fg_console. */ int vt_ioctl(struct tty_struct *tty, - unsigned int cmd, unsigned long arg) + unsigned int cmd, user_uintptr_t arg) { struct vc_data *vc = tty->driver_data; void __user *up = (void __user *)arg; @@ -845,7 +845,7 @@ int vt_ioctl(struct tty_struct *tty, return -ENXIO;
arg--; - arg = array_index_nospec(arg, MAX_NR_CONSOLES); + arg = array_index_nospec((unsigned long)arg, MAX_NR_CONSOLES); console_lock(); ret = vc_allocate(arg); console_unlock(); @@ -903,7 +903,7 @@ int vt_ioctl(struct tty_struct *tty, break; }
- arg = array_index_nospec(arg - 1, MAX_NR_CONSOLES); + arg = array_index_nospec((unsigned long)arg - 1, MAX_NR_CONSOLES); return vt_disallocate(arg);
case VT_RESIZE: diff --git a/include/linux/tty.h b/include/linux/tty.h index 730c3301d710..dd344a6f1bd0 100644 --- a/include/linux/tty.h +++ b/include/linux/tty.h @@ -458,7 +458,7 @@ int tty_set_termios(struct tty_struct *tty, struct ktermios *kt);
void tty_wakeup(struct tty_struct *tty);
-int tty_mode_ioctl(struct tty_struct *tty, unsigned int cmd, unsigned long arg); +int tty_mode_ioctl(struct tty_struct *tty, unsigned int cmd, user_uintptr_t arg); int tty_perform_flush(struct tty_struct *tty, unsigned long arg); struct tty_struct *tty_init_dev(struct tty_driver *driver, int idx); void tty_release_struct(struct tty_struct *tty, int idx); @@ -501,7 +501,7 @@ int n_tty_ioctl_helper(struct tty_struct *tty, unsigned int cmd,
/* vt.c */
-int vt_ioctl(struct tty_struct *tty, unsigned int cmd, unsigned long arg); +int vt_ioctl(struct tty_struct *tty, unsigned int cmd, user_uintptr_t arg);
long vt_compat_ioctl(struct tty_struct *tty, unsigned int cmd, unsigned long arg); diff --git a/include/linux/tty_driver.h b/include/linux/tty_driver.h index e00034118c7b..8301629c5c5d 100644 --- a/include/linux/tty_driver.h +++ b/include/linux/tty_driver.h @@ -363,7 +363,7 @@ struct tty_operations { unsigned int (*write_room)(struct tty_struct *tty); unsigned int (*chars_in_buffer)(struct tty_struct *tty); int (*ioctl)(struct tty_struct *tty, - unsigned int cmd, unsigned long arg); + unsigned int cmd, user_uintptr_t arg); long (*compat_ioctl)(struct tty_struct *tty, unsigned int cmd, unsigned long arg); void (*set_termios)(struct tty_struct *tty, const struct ktermios *old); diff --git a/include/linux/tty_ldisc.h b/include/linux/tty_ldisc.h index dcb61ec11424..b8c340a12bff 100644 --- a/include/linux/tty_ldisc.h +++ b/include/linux/tty_ldisc.h @@ -224,7 +224,7 @@ struct tty_ldisc_ops { ssize_t (*write)(struct tty_struct *tty, struct file *file, const unsigned char *buf, size_t nr); int (*ioctl)(struct tty_struct *tty, unsigned int cmd, - unsigned long arg); + user_uintptr_t arg); int (*compat_ioctl)(struct tty_struct *tty, unsigned int cmd, unsigned long arg); void (*set_termios)(struct tty_struct *tty, const struct ktermios *old); diff --git a/include/linux/vt_kern.h b/include/linux/vt_kern.h index c1f5aebef170..5cca5e106313 100644 --- a/include/linux/vt_kern.h +++ b/include/linux/vt_kern.h @@ -43,7 +43,7 @@ void redraw_screen(struct vc_data *vc, int is_switch); #define switch_screen(x) redraw_screen(x, 1)
struct tty_struct; -int tioclinux(struct tty_struct *tty, unsigned long arg); +int tioclinux(struct tty_struct *tty, user_uintptr_t arg);
#ifdef CONFIG_CONSOLE_TRANSLATIONS /* consolemap.c */ @@ -154,7 +154,7 @@ int vt_do_kbkeycode_ioctl(int cmd, struct kbkeycode __user *user_kbkc, int vt_do_kdsk_ioctl(int cmd, struct kbentry __user *user_kbe, int perm, unsigned int console); int vt_do_kdgkb_ioctl(int cmd, struct kbsentry __user *user_kdgkb, int perm); -int vt_do_kdskled(unsigned int console, int cmd, unsigned long arg, int perm); +int vt_do_kdskled(unsigned int console, int cmd, user_uintptr_t arg, int perm); int vt_do_kdgkbmode(unsigned int console); int vt_do_kdgkbmeta(unsigned int console); void vt_reset_unicode(unsigned int console);
The third argument of ioctl handlers is a user input which can contain either a pointer to data in the user space or any number. This commit updates the signature of ext4_ioctl_checkpoint, which is a child handler of __ext4_ioctl. __ext4_ioctl was updated to correctly accept and supply a user_uintptr_t but the signature of ext4_ioctl_checkpoint was not updated to reflect this change. Moreover the value of the argument is casted to unsigned long when outputted to the debug log to match the expected type.
Signed-off-by: Luca Vizzarro Luca.Vizzarro@arm.com --- fs/autofs/root.c | 2 +- fs/ext4/ioctl.c | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/fs/autofs/root.c b/fs/autofs/root.c index 554f8fb3947e..1f70683dc362 100644 --- a/fs/autofs/root.c +++ b/fs/autofs/root.c @@ -858,7 +858,7 @@ static int autofs_root_ioctl_unlocked(struct inode *inode, struct file *filp, void __user *p = (void __user *)arg;
pr_debug("cmd = 0x%08x, arg = 0x%08lx, sbi = %p, pgrp = %u\n", - cmd, arg, sbi, task_pgrp_nr(current)); + cmd, (unsigned long)arg, sbi, task_pgrp_nr(current));
if (_IOC_TYPE(cmd) != _IOC_TYPE(AUTOFS_IOC_FIRST) || _IOC_NR(cmd) - _IOC_NR(AUTOFS_IOC_FIRST) >= AUTOFS_IOC_COUNT) diff --git a/fs/ext4/ioctl.c b/fs/ext4/ioctl.c index 60437a1572ca..0d3ed9812e2b 100644 --- a/fs/ext4/ioctl.c +++ b/fs/ext4/ioctl.c @@ -1039,7 +1039,7 @@ static int ext4_ioctl_get_es_cache(struct file *filp, user_uintptr_t arg) return error; }
-static int ext4_ioctl_checkpoint(struct file *filp, unsigned long arg) +static int ext4_ioctl_checkpoint(struct file *filp, user_uintptr_t arg) { int err = 0; __u32 flags = 0; @@ -1215,7 +1215,7 @@ static long __ext4_ioctl(struct file *filp, unsigned int cmd, user_uintptr_t arg struct super_block *sb = inode->i_sb; struct user_namespace *mnt_userns = file_mnt_user_ns(filp);
- ext4_debug("cmd = %u, arg = %lu\n", cmd, arg); + ext4_debug("cmd = %u, arg = %lu\n", cmd, (unsigned long)arg);
switch (cmd) { case FS_IOC_GETFSMAP:
The third argument of ioctl handlers is a user input which can contain either a pointer to data in the user space or any number. This commit changes the type of the argument from unsigned long to user_uintptr_t so that we can accommodate the bigger size of capabilities and accept them from user space correctly without discarding their metadata.
Signed-off-by: Luca Vizzarro Luca.Vizzarro@arm.com --- include/linux/net.h | 2 +- include/net/inet_common.h | 2 +- include/net/ipv6.h | 2 +- include/net/sock.h | 2 +- include/net/tcp.h | 2 +- include/net/udp.h | 2 +- net/ipv4/af_inet.c | 2 +- net/ipv4/raw.c | 2 +- net/ipv4/tcp.c | 2 +- net/ipv4/udp.c | 2 +- net/ipv6/af_inet6.c | 2 +- net/ipv6/raw.c | 2 +- net/netlink/af_netlink.c | 2 +- net/packet/af_packet.c | 2 +- net/socket.c | 2 +- net/unix/af_unix.c | 4 ++-- 16 files changed, 17 insertions(+), 17 deletions(-)
diff --git a/include/linux/net.h b/include/linux/net.h index 18d942bbdf6e..43ee08cc57a7 100644 --- a/include/linux/net.h +++ b/include/linux/net.h @@ -176,7 +176,7 @@ struct proto_ops { __poll_t (*poll) (struct file *file, struct socket *sock, struct poll_table_struct *wait); int (*ioctl) (struct socket *sock, unsigned int cmd, - unsigned long arg); + user_uintptr_t arg); #ifdef CONFIG_COMPAT int (*compat_ioctl) (struct socket *sock, unsigned int cmd, unsigned long arg); diff --git a/include/net/inet_common.h b/include/net/inet_common.h index cec453c18f1d..e17bfdb8e5d2 100644 --- a/include/net/inet_common.h +++ b/include/net/inet_common.h @@ -53,7 +53,7 @@ int __inet_bind(struct sock *sk, struct sockaddr *uaddr, int addr_len, u32 flags); int inet_getname(struct socket *sock, struct sockaddr *uaddr, int peer); -int inet_ioctl(struct socket *sock, unsigned int cmd, unsigned long arg); +int inet_ioctl(struct socket *sock, unsigned int cmd, user_uintptr_t arg); int inet_ctl_sock_create(struct sock **sk, unsigned short family, unsigned short type, unsigned char protocol, struct net *net); diff --git a/include/net/ipv6.h b/include/net/ipv6.h index d383c895592a..6d301a05d66d 100644 --- a/include/net/ipv6.h +++ b/include/net/ipv6.h @@ -1188,7 +1188,7 @@ int inet6_release(struct socket *sock); int inet6_bind(struct socket *sock, struct sockaddr *uaddr, int addr_len); int inet6_getname(struct socket *sock, struct sockaddr *uaddr, int peer); -int inet6_ioctl(struct socket *sock, unsigned int cmd, unsigned long arg); +int inet6_ioctl(struct socket *sock, unsigned int cmd, user_uintptr_t arg); int inet6_compat_ioctl(struct socket *sock, unsigned int cmd, unsigned long arg);
diff --git a/include/net/sock.h b/include/net/sock.h index e0517ecc6531..153054819061 100644 --- a/include/net/sock.h +++ b/include/net/sock.h @@ -1242,7 +1242,7 @@ struct proto { bool kern);
int (*ioctl)(struct sock *sk, int cmd, - unsigned long arg); + user_uintptr_t arg); int (*init)(struct sock *sk); void (*destroy)(struct sock *sk); void (*shutdown)(struct sock *sk, int how); diff --git a/include/net/tcp.h b/include/net/tcp.h index 14d45661a84d..6c3498ce906b 100644 --- a/include/net/tcp.h +++ b/include/net/tcp.h @@ -342,7 +342,7 @@ void tcp_release_cb(struct sock *sk); void tcp_wfree(struct sk_buff *skb); void tcp_write_timer_handler(struct sock *sk); void tcp_delack_timer_handler(struct sock *sk); -int tcp_ioctl(struct sock *sk, int cmd, unsigned long arg); +int tcp_ioctl(struct sock *sk, int cmd, user_uintptr_t arg); int tcp_rcv_state_process(struct sock *sk, struct sk_buff *skb); void tcp_rcv_established(struct sock *sk, struct sk_buff *skb); void tcp_rcv_space_adjust(struct sock *sk); diff --git a/include/net/udp.h b/include/net/udp.h index fee053bcd17c..023a11ce421f 100644 --- a/include/net/udp.h +++ b/include/net/udp.h @@ -274,7 +274,7 @@ void udp_flush_pending_frames(struct sock *sk); int udp_cmsg_send(struct sock *sk, struct msghdr *msg, u16 *gso_size); void udp4_hwcsum(struct sk_buff *skb, __be32 src, __be32 dst); int udp_rcv(struct sk_buff *skb); -int udp_ioctl(struct sock *sk, int cmd, unsigned long arg); +int udp_ioctl(struct sock *sk, int cmd, user_uintptr_t arg); int udp_init_sock(struct sock *sk); int udp_pre_connect(struct sock *sk, struct sockaddr *uaddr, int addr_len); int __udp_disconnect(struct sock *sk, int flags); diff --git a/net/ipv4/af_inet.c b/net/ipv4/af_inet.c index 0da679411330..3968d555f496 100644 --- a/net/ipv4/af_inet.c +++ b/net/ipv4/af_inet.c @@ -932,7 +932,7 @@ EXPORT_SYMBOL(inet_shutdown); * There's a good 20K of config code hanging around the kernel. */
-int inet_ioctl(struct socket *sock, unsigned int cmd, unsigned long arg) +int inet_ioctl(struct socket *sock, unsigned int cmd, user_uintptr_t arg) { struct sock *sk = sock->sk; int err = 0; diff --git a/net/ipv4/raw.c b/net/ipv4/raw.c index 006c1f0ed8b4..86072fc11ffe 100644 --- a/net/ipv4/raw.c +++ b/net/ipv4/raw.c @@ -854,7 +854,7 @@ static int raw_getsockopt(struct sock *sk, int level, int optname, return do_raw_getsockopt(sk, level, optname, optval, optlen); }
-static int raw_ioctl(struct sock *sk, int cmd, unsigned long arg) +static int raw_ioctl(struct sock *sk, int cmd, user_uintptr_t arg) { switch (cmd) { case SIOCOUTQ: { diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c index 4f2205756cfe..17b213acffc8 100644 --- a/net/ipv4/tcp.c +++ b/net/ipv4/tcp.c @@ -595,7 +595,7 @@ __poll_t tcp_poll(struct file *file, struct socket *sock, poll_table *wait) } EXPORT_SYMBOL(tcp_poll);
-int tcp_ioctl(struct sock *sk, int cmd, unsigned long arg) +int tcp_ioctl(struct sock *sk, int cmd, user_uintptr_t arg) { struct tcp_sock *tp = tcp_sk(sk); int answ; diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c index 6a320a614e54..92d2da292071 100644 --- a/net/ipv4/udp.c +++ b/net/ipv4/udp.c @@ -1708,7 +1708,7 @@ static int first_packet_length(struct sock *sk) * IOCTL requests applicable to the UDP protocol */
-int udp_ioctl(struct sock *sk, int cmd, unsigned long arg) +int udp_ioctl(struct sock *sk, int cmd, user_uintptr_t arg) { switch (cmd) { case SIOCOUTQ: diff --git a/net/ipv6/af_inet6.c b/net/ipv6/af_inet6.c index 024191004982..2a429bb3f84a 100644 --- a/net/ipv6/af_inet6.c +++ b/net/ipv6/af_inet6.c @@ -566,7 +566,7 @@ int inet6_getname(struct socket *sock, struct sockaddr *uaddr, } EXPORT_SYMBOL(inet6_getname);
-int inet6_ioctl(struct socket *sock, unsigned int cmd, unsigned long arg) +int inet6_ioctl(struct socket *sock, unsigned int cmd, user_uintptr_t arg) { void __user *argp = (void __user *)arg; struct sock *sk = sock->sk; diff --git a/net/ipv6/raw.c b/net/ipv6/raw.c index 722de9dd0ff7..80fba26a0f8d 100644 --- a/net/ipv6/raw.c +++ b/net/ipv6/raw.c @@ -1114,7 +1114,7 @@ static int rawv6_getsockopt(struct sock *sk, int level, int optname, return do_rawv6_getsockopt(sk, level, optname, optval, optlen); }
-static int rawv6_ioctl(struct sock *sk, int cmd, unsigned long arg) +static int rawv6_ioctl(struct sock *sk, int cmd, user_uintptr_t arg) { switch (cmd) { case SIOCOUTQ: { diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c index a662e8a5ff84..46ca0da083bc 100644 --- a/net/netlink/af_netlink.c +++ b/net/netlink/af_netlink.c @@ -1139,7 +1139,7 @@ static int netlink_getname(struct socket *sock, struct sockaddr *addr, }
static int netlink_ioctl(struct socket *sock, unsigned int cmd, - unsigned long arg) + user_uintptr_t arg) { /* try to hand this ioctl down to the NIC drivers. */ diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c index 1ab65f7f2a0a..a7e2a3a145b3 100644 --- a/net/packet/af_packet.c +++ b/net/packet/af_packet.c @@ -4188,7 +4188,7 @@ static int packet_notifier(struct notifier_block *this,
static int packet_ioctl(struct socket *sock, unsigned int cmd, - unsigned long arg) + user_uintptr_t arg) { struct sock *sk = sock->sk;
diff --git a/net/socket.c b/net/socket.c index 7279bb1a4c28..1238bd1560ce 100644 --- a/net/socket.c +++ b/net/socket.c @@ -1158,7 +1158,7 @@ void vlan_ioctl_set(int (*hook) (struct net *, void __user *)) EXPORT_SYMBOL(vlan_ioctl_set);
static long sock_do_ioctl(struct net *net, struct socket *sock, - unsigned int cmd, unsigned long arg) + unsigned int cmd, user_uintptr_t arg) { struct ifreq ifr; bool need_copyout; diff --git a/net/unix/af_unix.c b/net/unix/af_unix.c index ac90bba7cbb1..5b4671f503a8 100644 --- a/net/unix/af_unix.c +++ b/net/unix/af_unix.c @@ -750,7 +750,7 @@ static int unix_getname(struct socket *, struct sockaddr *, int); static __poll_t unix_poll(struct file *, struct socket *, poll_table *); static __poll_t unix_dgram_poll(struct file *, struct socket *, poll_table *); -static int unix_ioctl(struct socket *, unsigned int, unsigned long); +static int unix_ioctl(struct socket *, unsigned int, user_uintptr_t); #ifdef CONFIG_COMPAT static int unix_compat_ioctl(struct socket *sock, unsigned int cmd, unsigned long arg); #endif @@ -3094,7 +3094,7 @@ static int unix_open_file(struct sock *sk) return fd; }
-static int unix_ioctl(struct socket *sock, unsigned int cmd, unsigned long arg) +static int unix_ioctl(struct socket *sock, unsigned int cmd, user_uintptr_t arg) { struct sock *sk = sock->sk; long amount = 0;
On 02/02/2023 17:50, Luca Vizzarro wrote:
|diff --git a/net/socket.c b/net/socket.c index 7279bb1a4c28..1238bd1560ce 100644 --- a/net/socket.c +++ b/net/socket.c @@ -1158,7 +1158,7 @@ void vlan_ioctl_set(int (*hook) (struct net *, void __user *)) EXPORT_SYMBOL(vlan_ioctl_set); static long sock_do_ioctl(struct net *net, struct socket *sock, - unsigned int cmd, unsigned long arg) + unsigned int cmd, user_uintptr_t arg)|
This patch looks all good, but like the first patch, trying to follow both the native and compat sides suggests there's an issue with the top-level compat handler, specifically compat_sock_ioctl_trans() in this file. For most commands, it passes the unsigned long arg directly to the native handler, and it looks like most commands expect a pointer.
Ideally we would only use compat_ptr() for the commands where the argument is actually a pointer, but if that proves too complicated, using it all for all the remaining commands would be acceptable.
Kevin
The function br_dev_read_uargs copies a list of arguments from userspace. Normally, they may contain pointers or real numbers. When working with capabilities, the type unsigned long used for this purpose is not big enough to hold them. This commit changes the type of args from unsigned long to user_uintptr_t for this purpose. As a consequence the type of the variables fed to this function is also updated, and casted to unsigned long when treated as a number. It also fixes the type of argp, as it points to an entry of args, which lives in the kernel space, and not in the user space as the type suggests.
Signed-off-by: Luca Vizzarro Luca.Vizzarro@arm.com --- net/bridge/br_ioctl.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/net/bridge/br_ioctl.c b/net/bridge/br_ioctl.c index 9c3030de438c..aa4e34cf8754 100644 --- a/net/bridge/br_ioctl.c +++ b/net/bridge/br_ioctl.c @@ -104,9 +104,9 @@ static int add_del_if(struct net_bridge *br, int ifindex, int isadd) }
#define BR_UARGS_MAX 4 -static int br_dev_read_uargs(unsigned long *args, size_t nr_args, +static int br_dev_read_uargs(user_uintptr_t *args, size_t nr_args, #ifdef CONFIG_CHERI_PURECAP_UABI - void * __capability * __capability argp, void __user *data) + void * __capability *argp, void __user *data) #else void __user **argp, void __user *data) #endif @@ -150,7 +150,7 @@ int br_dev_siocdevprivate(struct net_device *dev, struct ifreq *rq, { struct net_bridge *br = netdev_priv(dev); struct net_bridge_port *p = NULL; - unsigned long args[4]; + user_uintptr_t args[4]; void __user *argp; int ret;
@@ -342,7 +342,7 @@ int br_dev_siocdevprivate(struct net_device *dev, struct ifreq *rq,
static int old_deviceless(struct net *net, void __user *data) { - unsigned long args[3]; + user_uintptr_t args[3]; void __user *argp; int ret;
@@ -369,7 +369,7 @@ static int old_deviceless(struct net *net, void __user *data)
ret = copy_to_user(argp, indices, array_size(args[2], sizeof(int))) - ? -EFAULT : args[2]; + ? -EFAULT : (unsigned long)args[2];
kfree(indices); return ret;
On 02/02/2023 17:50, Luca Vizzarro wrote:
The function br_dev_read_uargs copies a list of arguments from userspace. Normally, they may contain pointers or real numbers. When working with capabilities, the type unsigned long used for this purpose is not big enough to hold them. This commit changes the type of args from unsigned long to user_uintptr_t for this purpose. As a consequence the type of the variables fed to this function is also updated, and casted to unsigned long when treated as a number. It also fixes the type of argp, as it points to an entry of args, which lives in the kernel space, and not in the user space as the type suggests.
That is a good catch! I'm really surprised Clang accepted this without warning... This fixup was introduced by a previous commit, so you could add:
Fixes: ("net: Fix the position of __capability in double pointer")
Signed-off-by: Luca Vizzarro Luca.Vizzarro@arm.com
net/bridge/br_ioctl.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/net/bridge/br_ioctl.c b/net/bridge/br_ioctl.c index 9c3030de438c..aa4e34cf8754 100644 --- a/net/bridge/br_ioctl.c +++ b/net/bridge/br_ioctl.c @@ -104,9 +104,9 @@ static int add_del_if(struct net_bridge *br, int ifindex, int isadd) } #define BR_UARGS_MAX 4 -static int br_dev_read_uargs(unsigned long *args, size_t nr_args, +static int br_dev_read_uargs(user_uintptr_t *args, size_t nr_args,
This made me pause as it's a change in ABI. All things considered I thing it makes sense, that's the most coherent way to approach it. Fortunately it should impact very little software, especially since this is only used for old APIs and most software only uses them as fallback. Still, I found at least one case (toybox [1]) where only the old APIs are used, so this may not go entirely unnoticed (unsigned long will need to be replaced with uintptr_t for the array and casts). I'll add this to the PCuABI spec.
On a more practical note, you need an additional change if you start reading user pointers from memory this way: the copy_from_user() below needs to become copy_from_user_with_ptr() to preserve capability tags.
[1] https://github.com/landley/toybox/blob/master/toys/pending/brctl.c
#ifdef CONFIG_CHERI_PURECAP_UABI
void * __capability * __capability argp, void __user *data)
void * __capability *argp, void __user *data)#else void __user **argp, void __user *data) #endif @@ -150,7 +150,7 @@ int br_dev_siocdevprivate(struct net_device *dev, struct ifreq *rq, { struct net_bridge *br = netdev_priv(dev); struct net_bridge_port *p = NULL;
- unsigned long args[4];
- user_uintptr_t args[4]; void __user *argp; int ret;
@@ -342,7 +342,7 @@ int br_dev_siocdevprivate(struct net_device *dev, struct ifreq *rq, static int old_deviceless(struct net *net, void __user *data) {
- unsigned long args[3];
- user_uintptr_t args[3]; void __user *argp; int ret;
@@ -369,7 +369,7 @@ static int old_deviceless(struct net *net, void __user *data) ret = copy_to_user(argp, indices, array_size(args[2], sizeof(int)))
? -EFAULT : args[2];
? -EFAULT : (unsigned long)args[2];
int surely? Both sides of the ternary should have the same type (and ret is an int), I'm surprised the compiler doesn't complain about that.
Kevin
kfree(indices); return ret;
The br_dev_read_uargs function copies a list of arguments from a user capability. When operating in compat64, pointers are 64-bit long and cannot be represented as unsigned int. In this scenario the function copies a list of compat pointers, therefore the type of the receiver variable, cargs, must be updated to compat_uptr_t from unsigned int.
Signed-off-by: Luca Vizzarro Luca.Vizzarro@arm.com --- net/bridge/br_ioctl.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/net/bridge/br_ioctl.c b/net/bridge/br_ioctl.c index aa4e34cf8754..439a5e5d7b5d 100644 --- a/net/bridge/br_ioctl.c +++ b/net/bridge/br_ioctl.c @@ -117,7 +117,7 @@ static int br_dev_read_uargs(user_uintptr_t *args, size_t nr_args, return -EINVAL;
if (in_compat_syscall()) { - unsigned int cargs[BR_UARGS_MAX]; + compat_uptr_t cargs[BR_UARGS_MAX]; int i;
ret = copy_from_user(cargs, data, nr_args * sizeof(*cargs));
Nit: in commit title, "net: bridge:" seems to be the most common tag for this file (same for the previous patch).
Kevin
On 02/02/2023 17:50, Luca Vizzarro wrote:
The br_dev_read_uargs function copies a list of arguments from a user capability. When operating in compat64, pointers are 64-bit long and cannot be represented as unsigned int. In this scenario the function copies a list of compat pointers, therefore the type of the receiver variable, cargs, must be updated to compat_uptr_t from unsigned int.
Signed-off-by: Luca Vizzarro Luca.Vizzarro@arm.com
net/bridge/br_ioctl.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/net/bridge/br_ioctl.c b/net/bridge/br_ioctl.c index aa4e34cf8754..439a5e5d7b5d 100644 --- a/net/bridge/br_ioctl.c +++ b/net/bridge/br_ioctl.c @@ -117,7 +117,7 @@ static int br_dev_read_uargs(user_uintptr_t *args, size_t nr_args, return -EINVAL; if (in_compat_syscall()) {
unsigned int cargs[BR_UARGS_MAX];
compat_uptr_t cargs[BR_UARGS_MAX];ret = copy_from_user(cargs, data, nr_args * sizeof(*cargs));
The posix_clock_ioctl handler mostly takes a user pointer as an argument, and the compat implementation is not PCuABI compatible.
This commit removes the posix_clock_compat_ioctl handler and replaces it with the compat_ptr_ioctl helper function instead.
Signed-off-by: Luca Vizzarro Luca.Vizzarro@arm.com --- kernel/time/posix-clock.c | 21 +-------------------- 1 file changed, 1 insertion(+), 20 deletions(-)
diff --git a/kernel/time/posix-clock.c b/kernel/time/posix-clock.c index 8ecfea067832..09fd22d50bc3 100644 --- a/kernel/time/posix-clock.c +++ b/kernel/time/posix-clock.c @@ -86,25 +86,6 @@ static long posix_clock_ioctl(struct file *fp, return err; }
-#ifdef CONFIG_COMPAT -static long posix_clock_compat_ioctl(struct file *fp, - unsigned int cmd, unsigned long arg) -{ - struct posix_clock *clk = get_posix_clock(fp); - int err = -ENOTTY; - - if (!clk) - return -ENODEV; - - if (clk->ops.ioctl) - err = clk->ops.ioctl(clk, cmd, arg); - - put_posix_clock(clk); - - return err; -} -#endif - static int posix_clock_open(struct inode *inode, struct file *fp) { int err; @@ -155,7 +136,7 @@ static const struct file_operations posix_clock_file_operations = { .open = posix_clock_open, .release = posix_clock_release, #ifdef CONFIG_COMPAT - .compat_ioctl = posix_clock_compat_ioctl, + .compat_ioctl = compat_ptr_ioctl, #endif };
Nit: in commit title, the most common tag seems to be "posix-clocks:".
On 02/02/2023 17:50, Luca Vizzarro wrote:
The posix_clock_ioctl handler mostly takes a user pointer as an argument, and the compat implementation is not PCuABI compatible.
This commit removes the posix_clock_compat_ioctl handler and replaces it with the compat_ptr_ioctl helper function instead.
Signed-off-by: Luca Vizzarro Luca.Vizzarro@arm.com
kernel/time/posix-clock.c | 21 +-------------------- 1 file changed, 1 insertion(+), 20 deletions(-)
diff --git a/kernel/time/posix-clock.c b/kernel/time/posix-clock.c index 8ecfea067832..09fd22d50bc3 100644 --- a/kernel/time/posix-clock.c +++ b/kernel/time/posix-clock.c @@ -86,25 +86,6 @@ static long posix_clock_ioctl(struct file *fp, return err; } -#ifdef CONFIG_COMPAT -static long posix_clock_compat_ioctl(struct file *fp,
unsigned int cmd, unsigned long arg)-{
- struct posix_clock *clk = get_posix_clock(fp);
- int err = -ENOTTY;
- if (!clk)
return -ENODEV;- if (clk->ops.ioctl)
err = clk->ops.ioctl(clk, cmd, arg);- put_posix_clock(clk);
- return err;
-} -#endif
static int posix_clock_open(struct inode *inode, struct file *fp) { int err; @@ -155,7 +136,7 @@ static const struct file_operations posix_clock_file_operations = { .open = posix_clock_open, .release = posix_clock_release, #ifdef CONFIG_COMPAT
Good idea to replace the compat handler with compat_ptr_ioctl, not sure why they duplicated the implementation... While at it you could also remove the #ifdef, compat_ptr_ioctl is already #ifdef'd as needed.
Last minor thing, it would make more sense to have this patch just after patch 1, since they are related.
Kevin
- .compat_ioctl = posix_clock_compat_ioctl,
- .compat_ioctl = compat_ptr_ioctl,
#endif };
On 02/02/2023 17:50, Luca Vizzarro wrote:
Hi all,
This patch series addresses the issue reported in the following issue: https://git.morello-project.org/morello/kernel/linux/-/issues/45
Changes available at: https://git.morello-project.org/Sevenarth/linux/-/commits/morello/6.1-ioctl-...
v2:
- discarded the posix_clock_compat_ioctl handler and replaced with the compat_ptr_ioctl helper function
Luca Vizzarro (7): ptp: Modify ptp_ioctl to use user_uintptr_t tty: Modify ioctl to use user_uintptr_t fs: Fix user_uinptr_t behaviour with ioctl net: Modify ioctl to use user_uintptr_t net/br_ioctl: Modify br_dev_read_uargs to accept user_uintptr_t net/br_ioctl: Support 64-bit pointers in compat time/posix_clock: Use compat_ptr_ioctl helper
Very good series, including the commit messages and the way the patches are split, I'm impressed! Just a few small issues and another compat handler that needs attention (patch 4).
Kevin
drivers/ptp/ptp_chardev.c | 2 +- drivers/ptp/ptp_private.h | 2 +- drivers/tty/n_tty.c | 2 +- drivers/tty/pty.c | 4 ++-- drivers/tty/serial/serial_core.c | 2 +- drivers/tty/tty_ioctl.c | 2 +- drivers/tty/vt/keyboard.c | 2 +- drivers/tty/vt/vt.c | 2 +- drivers/tty/vt/vt_ioctl.c | 8 ++++---- fs/autofs/root.c | 2 +- fs/ext4/ioctl.c | 4 ++-- include/linux/net.h | 2 +- include/linux/posix-clock.h | 2 +- include/linux/tty.h | 4 ++-- include/linux/tty_driver.h | 2 +- include/linux/tty_ldisc.h | 2 +- include/linux/vt_kern.h | 4 ++-- include/net/inet_common.h | 2 +- include/net/ipv6.h | 2 +- include/net/sock.h | 2 +- include/net/tcp.h | 2 +- include/net/udp.h | 2 +- kernel/time/posix-clock.c | 21 +-------------------- net/bridge/br_ioctl.c | 12 ++++++------ net/ipv4/af_inet.c | 2 +- net/ipv4/raw.c | 2 +- net/ipv4/tcp.c | 2 +- net/ipv4/udp.c | 2 +- net/ipv6/af_inet6.c | 2 +- net/ipv6/raw.c | 2 +- net/netlink/af_netlink.c | 2 +- net/packet/af_packet.c | 2 +- net/socket.c | 2 +- net/unix/af_unix.c | 4 ++-- 34 files changed, 47 insertions(+), 66 deletions(-)
linux-morello@op-lists.linaro.org
-
Kevin Brodsky -
Luca Vizzarro