| Message ID | 20211223153139.7661-3-repk@triplefau.lt (mailing list archive) |
|---|---|
| State | Deferred |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Fix SIOCGIFBR/SIOCSIFBR ioctl | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Clearly marked for net |
| netdev/fixes_present | success | Fixes tag present in non-next series |
| netdev/subject_prefix | success | Link |
| netdev/cover_letter | success | Series has a cover letter |
| netdev/patch_count | success | Link |
| netdev/header_inline | success | No static functions without inline keyword in header files |
| netdev/build_32bit | success | Errors and warnings before: 2 this patch: 2 |
| netdev/cc_maintainers | success | CCed 6 of 6 maintainers |
| netdev/build_clang | success | Errors and warnings before: 20 this patch: 20 |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Signed-off-by tag matches author and committer |
| netdev/verify_fixes | success | No Fixes tag |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 4 this patch: 4 |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 157 lines checked |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/source_inline | success | Was 0 now: 0 |
On Thu, 23 Dec 2021 16:31:39 +0100 Remi Pommarel wrote: > In compat mode SIOC{G,S}IFBR ioctls were only supporting > BRCTL_GET_VERSION returning an artificially version to spur userland > tool to use SIOCDEVPRIVATE instead. But some userland tools ignore that > and use SIOC{G,S}IFBR unconditionally as seen with busybox's brctl. > > Example of non working 32-bit brctl with CONFIG_COMPAT=y: > $ brctl show > brctl: SIOCGIFBR: Invalid argument > > Example of fixed 32-bit brctl with CONFIG_COMPAT=y: > $ brctl show > bridge name bridge id STP enabled interfaces > br0 > > Signed-off-by: Remi Pommarel <repk@triplefau.lt> > Co-developed-by: Arnd Bergmann <arnd@arndb.de> > Signed-off-by: Arnd Bergmann <arnd@arndb.de> Since Arnd said this is not supposed to be backported I presume it should go to net-next?
On Thu, Dec 23, 2021 at 08:59:44AM -0800, Jakub Kicinski wrote: > On Thu, 23 Dec 2021 16:31:39 +0100 Remi Pommarel wrote: > > In compat mode SIOC{G,S}IFBR ioctls were only supporting > > BRCTL_GET_VERSION returning an artificially version to spur userland > > tool to use SIOCDEVPRIVATE instead. But some userland tools ignore that > > and use SIOC{G,S}IFBR unconditionally as seen with busybox's brctl. > > > > Example of non working 32-bit brctl with CONFIG_COMPAT=y: > > $ brctl show > > brctl: SIOCGIFBR: Invalid argument > > > > Example of fixed 32-bit brctl with CONFIG_COMPAT=y: > > $ brctl show > > bridge name bridge id STP enabled interfaces > > br0 > > > > Signed-off-by: Remi Pommarel <repk@triplefau.lt> > > Co-developed-by: Arnd Bergmann <arnd@arndb.de> > > Signed-off-by: Arnd Bergmann <arnd@arndb.de> > > Since Arnd said this is not supposed to be backported I presume it > should go to net-next? Yes, out of curiosity, is it appropriate to mix "[PATCH net]" and "[PATCH net-next]" in the same serie ? Thanks
On Thu, 23 Dec 2021 18:50:30 +0100 Remi Pommarel wrote: > On Thu, Dec 23, 2021 at 08:59:44AM -0800, Jakub Kicinski wrote: > > On Thu, 23 Dec 2021 16:31:39 +0100 Remi Pommarel wrote: > > > In compat mode SIOC{G,S}IFBR ioctls were only supporting > > > BRCTL_GET_VERSION returning an artificially version to spur userland > > > tool to use SIOCDEVPRIVATE instead. But some userland tools ignore that > > > and use SIOC{G,S}IFBR unconditionally as seen with busybox's brctl. > > > > > > Example of non working 32-bit brctl with CONFIG_COMPAT=y: > > > $ brctl show > > > brctl: SIOCGIFBR: Invalid argument > > > > > > Example of fixed 32-bit brctl with CONFIG_COMPAT=y: > > > $ brctl show > > > bridge name bridge id STP enabled interfaces > > > br0 > > > > > > Signed-off-by: Remi Pommarel <repk@triplefau.lt> > > > Co-developed-by: Arnd Bergmann <arnd@arndb.de> > > > Signed-off-by: Arnd Bergmann <arnd@arndb.de> > > > > Since Arnd said this is not supposed to be backported I presume it > > should go to net-next? > > Yes, out of curiosity, is it appropriate to mix "[PATCH net]" and > "[PATCH net-next]" in the same serie ? It's not, mixing makes it quite hard to know what's needed where. Also hard to automate things on our end. Let me pick out the first patch, I'll be sending a PR to Linus shortly and then merge net into net-next. At which point you'll be able to rebase on top of net-next and resend just the second patch for net-next..
On Thu, Dec 23, 2021 at 09:53:18AM -0800, Jakub Kicinski wrote: > On Thu, 23 Dec 2021 18:50:30 +0100 Remi Pommarel wrote: > > On Thu, Dec 23, 2021 at 08:59:44AM -0800, Jakub Kicinski wrote: > > > On Thu, 23 Dec 2021 16:31:39 +0100 Remi Pommarel wrote: > > > > In compat mode SIOC{G,S}IFBR ioctls were only supporting > > > > BRCTL_GET_VERSION returning an artificially version to spur userland > > > > tool to use SIOCDEVPRIVATE instead. But some userland tools ignore that > > > > and use SIOC{G,S}IFBR unconditionally as seen with busybox's brctl. > > > > > > > > Example of non working 32-bit brctl with CONFIG_COMPAT=y: > > > > $ brctl show > > > > brctl: SIOCGIFBR: Invalid argument > > > > > > > > Example of fixed 32-bit brctl with CONFIG_COMPAT=y: > > > > $ brctl show > > > > bridge name bridge id STP enabled interfaces > > > > br0 > > > > > > > > Signed-off-by: Remi Pommarel <repk@triplefau.lt> > > > > Co-developed-by: Arnd Bergmann <arnd@arndb.de> > > > > Signed-off-by: Arnd Bergmann <arnd@arndb.de> > > > > > > Since Arnd said this is not supposed to be backported I presume it > > > should go to net-next? > > > > Yes, out of curiosity, is it appropriate to mix "[PATCH net]" and > > "[PATCH net-next]" in the same serie ? > > It's not, mixing makes it quite hard to know what's needed where. > Also hard to automate things on our end. Let me pick out the first > patch, I'll be sending a PR to Linus shortly and then merge net into > net-next. At which point you'll be able to rebase on top of net-next > and resend just the second patch for net-next.. Ok sounds good to me thanks.
diff --git a/net/bridge/br_ioctl.c b/net/bridge/br_ioctl.c index 891cfcf45644..5f3c1cf7f6ad 100644 --- a/net/bridge/br_ioctl.c +++ b/net/bridge/br_ioctl.c @@ -102,37 +102,56 @@ static int add_del_if(struct net_bridge *br, int ifindex, int isadd) return ret; } +#define BR_UARGS_MAX 4 +static int br_dev_read_uargs(unsigned long *args, size_t nr_args, + void __user **argp, void __user *data) +{ + int ret; + + if (nr_args < 2 || nr_args > BR_UARGS_MAX) + return -EINVAL; + + if (in_compat_syscall()) { + unsigned int cargs[BR_UARGS_MAX]; + int i; + + ret = copy_from_user(cargs, data, nr_args * sizeof(*cargs)); + if (ret) + goto fault; + + for (i = 0; i < nr_args; ++i) + args[i] = cargs[i]; + + *argp = compat_ptr(args[1]); + } else { + ret = copy_from_user(args, data, nr_args * sizeof(*args)); + if (ret) + goto fault; + *argp = (void __user *)args[1]; + } + + return 0; +fault: + return -EFAULT; +} + /* * Legacy ioctl's through SIOCDEVPRIVATE * This interface is deprecated because it was too difficult * to do the translation for 32/64bit ioctl compatibility. */ -int br_dev_siocdevprivate(struct net_device *dev, struct ifreq *rq, void __user *data, int cmd) +int br_dev_siocdevprivate(struct net_device *dev, struct ifreq *rq, + void __user *data, int cmd) { struct net_bridge *br = netdev_priv(dev); struct net_bridge_port *p = NULL; unsigned long args[4]; void __user *argp; - int ret = -EOPNOTSUPP; - - if (in_compat_syscall()) { - unsigned int cargs[4]; - - if (copy_from_user(cargs, data, sizeof(cargs))) - return -EFAULT; - - args[0] = cargs[0]; - args[1] = cargs[1]; - args[2] = cargs[2]; - args[3] = cargs[3]; - - argp = compat_ptr(args[1]); - } else { - if (copy_from_user(args, data, sizeof(args))) - return -EFAULT; + int ret; - argp = (void __user *)args[1]; - } + ret = br_dev_read_uargs(args, ARRAY_SIZE(args), &argp, data); + if (ret) + return ret; switch (args[0]) { case BRCTL_ADD_IF: @@ -301,6 +320,9 @@ int br_dev_siocdevprivate(struct net_device *dev, struct ifreq *rq, void __user case BRCTL_GET_FDB_ENTRIES: return get_fdb_entries(br, argp, args[2], args[3]); + + default: + ret = -EOPNOTSUPP; } if (!ret) { @@ -313,12 +335,15 @@ int br_dev_siocdevprivate(struct net_device *dev, struct ifreq *rq, void __user return ret; } -static int old_deviceless(struct net *net, void __user *uarg) +static int old_deviceless(struct net *net, void __user *data) { unsigned long args[3]; + void __user *argp; + int ret; - if (copy_from_user(args, uarg, sizeof(args))) - return -EFAULT; + ret = br_dev_read_uargs(args, ARRAY_SIZE(args), &argp, data); + if (ret) + return ret; switch (args[0]) { case BRCTL_GET_VERSION: @@ -337,7 +362,7 @@ static int old_deviceless(struct net *net, void __user *uarg) args[2] = get_bridge_ifindices(net, indices, args[2]); - ret = copy_to_user((void __user *)args[1], indices, + ret = copy_to_user(argp, indices, array_size(args[2], sizeof(int))) ? -EFAULT : args[2]; @@ -353,7 +378,7 @@ static int old_deviceless(struct net *net, void __user *uarg) if (!ns_capable(net->user_ns, CAP_NET_ADMIN)) return -EPERM; - if (copy_from_user(buf, (void __user *)args[1], IFNAMSIZ)) + if (copy_from_user(buf, argp, IFNAMSIZ)) return -EFAULT; buf[IFNAMSIZ-1] = 0; diff --git a/net/socket.c b/net/socket.c index 7f64a6eccf63..6b2a898055ca 100644 --- a/net/socket.c +++ b/net/socket.c @@ -3233,21 +3233,6 @@ static int compat_ifr_data_ioctl(struct net *net, unsigned int cmd, return dev_ioctl(net, cmd, &ifreq, data, NULL); } -/* Since old style bridge ioctl's endup using SIOCDEVPRIVATE - * for some operations; this forces use of the newer bridge-utils that - * use compatible ioctls - */ -static int old_bridge_ioctl(compat_ulong_t __user *argp) -{ - compat_ulong_t tmp; - - if (get_user(tmp, argp)) - return -EFAULT; - if (tmp == BRCTL_GET_VERSION) - return BRCTL_VERSION + 1; - return -EINVAL; -} - static int compat_sock_ioctl_trans(struct file *file, struct socket *sock, unsigned int cmd, unsigned long arg) { @@ -3259,9 +3244,6 @@ static int compat_sock_ioctl_trans(struct file *file, struct socket *sock, return sock_ioctl(file, cmd, (unsigned long)argp); switch (cmd) { - case SIOCSIFBR: - case SIOCGIFBR: - return old_bridge_ioctl(argp); case SIOCWANDEV: return compat_siocwandev(net, argp); case SIOCGSTAMP_OLD: @@ -3290,6 +3272,8 @@ static int compat_sock_ioctl_trans(struct file *file, struct socket *sock, case SIOCGSTAMP_NEW: case SIOCGSTAMPNS_NEW: case SIOCGIFCONF: + case SIOCSIFBR: + case SIOCGIFBR: return sock_ioctl(file, cmd, arg); case SIOCGIFFLAGS:
In compat mode SIOC{G,S}IFBR ioctls were only supporting BRCTL_GET_VERSION returning an artificially version to spur userland tool to use SIOCDEVPRIVATE instead. But some userland tools ignore that and use SIOC{G,S}IFBR unconditionally as seen with busybox's brctl. Example of non working 32-bit brctl with CONFIG_COMPAT=y: $ brctl show brctl: SIOCGIFBR: Invalid argument Example of fixed 32-bit brctl with CONFIG_COMPAT=y: $ brctl show bridge name bridge id STP enabled interfaces br0 Signed-off-by: Remi Pommarel <repk@triplefau.lt> Co-developed-by: Arnd Bergmann <arnd@arndb.de> Signed-off-by: Arnd Bergmann <arnd@arndb.de> --- net/bridge/br_ioctl.c | 75 ++++++++++++++++++++++++++++--------------- net/socket.c | 20 ++---------- 2 files changed, 52 insertions(+), 43 deletions(-)