Remi Pommarel
2021-Dec-23 15:31 UTC
[Bridge] [PATCH net 0/2] Fix SIOCGIFBR/SIOCSIFBR ioctl
SIOC{G,S}IFBR ioctls have been broken since [0], as discussed here [1]
the intent was to get them working in compat mode.
This serie is gathering patch from [2] with the one from [3].
The first patch fixes the ioctl usage so it can be backported to stable
kernel while the second one adds proper support for those ioctl in
compat mode.
This has been tested with busybox's brctl as below.
Before this serie
- 64-bit brctl:
$ brctl show
bridge name bridge id STP enabled interfaces
brctl: can't get bridge name for index 0: No such device or address
- 32-bit brctl on CONFIG_COMPAT=y kernel:
$ brctl show
brctl: SIOCGIFBR: Invalid argument
With first patch of this serie
- 64-bit brctl
$ brctl show
bridge name bridge id STP enabled interfaces
br0 8000.000000000000 no
- 32-bit brctl on CONFIG_COMPAT=y kernel
$ brctl show
brctl: SIOCGIFBR: Invalid argument
With both patches
- 64-bit brctl
$ brctl show
bridge name bridge id STP enabled interfaces
br0 8000.000000000000 no
- 32-bit brctl on CONFIG_COMPAT=y kernel
$ brctl show
bridge name bridge id STP enabled interfaces
br0 8000.000000000000 no
[0] commit 561d8352818f ("bridge: use ndo_siocdevprivate")
[1] https://lkml.org/lkml/2021/12/22/805
[2] https://lkml.org/lkml/2021/12/22/743
[3] https://lkml.org/lkml/2021/12/23/212
Thanks,
Remi Pommarel (2):
net: bridge: fix ioctl old_deviceless bridge argument
net: bridge: Get SIOCGIFBR/SIOCSIFBR ioctl working in compat mode
net/bridge/br_ioctl.c | 75 ++++++++++++++++++++++++++++---------------
net/socket.c | 20 ++----------
2 files changed, 52 insertions(+), 43 deletions(-)
--
2.33.0
Remi Pommarel
2021-Dec-23 15:31 UTC
[Bridge] [PATCH net 1/2] net: bridge: fix ioctl old_deviceless bridge argument
Commit 561d8352818f ("bridge: use ndo_siocdevprivate") changed the
source and destination arguments of copy_{to,from}_user in bridge's
old_deviceless() from args[1] to uarg breaking SIOC{G,S}IFBR ioctls.
Commit cbd7ad29a507 ("net: bridge: fix ioctl old_deviceless bridge
argument") fixed only BRCTL_{ADD,DEL}_BRIDGES commands leaving
BRCTL_GET_BRIDGES one untouched.
The fixes BRCTL_GET_BRIDGES as well and has been tested with busybox's
brctl.
Example of broken brctl:
$ brctl show
bridge name bridge id STP enabled interfaces
brctl: can't get bridge name for index 0: No such device or address
Example of fixed brctl:
$ brctl show
bridge name bridge id STP enabled interfaces
br0 8000.000000000000 no
Fixes: 561d8352818f ("bridge: use ndo_siocdevprivate")
Signed-off-by: Remi Pommarel <repk at triplefau.lt>
Reviewed-by: Arnd Bergmann <arnd at arndb.de>
Acked-by: Nikolay Aleksandrov <nikolay at nvidia.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 db4ab2c2ce18..891cfcf45644 100644
--- a/net/bridge/br_ioctl.c
+++ b/net/bridge/br_ioctl.c
@@ -337,7 +337,7 @@ static int old_deviceless(struct net *net, void __user
*uarg)
args[2] = get_bridge_ifindices(net, indices, args[2]);
- ret = copy_to_user(uarg, indices,
+ ret = copy_to_user((void __user *)args[1], indices,
array_size(args[2], sizeof(int)))
? -EFAULT : args[2];
--
2.33.0
Remi Pommarel
2021-Dec-23 15:31 UTC
[Bridge] [PATCH net 2/2] net: bridge: Get SIOCGIFBR/SIOCSIFBR ioctl working in compat mode
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 at triplefau.lt>
Co-developed-by: Arnd Bergmann <arnd at arndb.de>
Signed-off-by: Arnd Bergmann <arnd at arndb.de>
---
net/bridge/br_ioctl.c | 75 ++++++++++++++++++++++++++++---------------
net/socket.c | 20 ++----------
2 files changed, 52 insertions(+), 43 deletions(-)
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:
--
2.33.0