[PATCH net] net: bridge: fix ioctl old_deviceless bridge argument

[PATCH net] net: bridge: fix ioctl old_deviceless bridge argument

[Remi Pommarel]

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

Fixes: 561d8352818f ("bridge: use ndo_siocdevprivate")
Signed-off-by: Remi Pommarel <repk@triplefau.lt>
---
 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

Re: [PATCH net] net: bridge: fix ioctl old_deviceless bridge argument

[Arnd Bergmann]

On Wed, Dec 22, 2021 at 8:13 PM Remi Pommarel <repk@triplefau.lt> wrote:
>
> 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
>
> Fixes: 561d8352818f ("bridge: use ndo_siocdevprivate")
> Signed-off-by: Remi Pommarel <repk@triplefau.lt>

Thanks for fixing the regression,

Reviewed-by: Arnd Bergmann <arnd@arndb.de>

> ---
>  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];

The intention of my broken patch was to make it work for compat mode as I did
in br_dev_siocdevprivate(), as this is now the only bit that remains broken.

This could be done along the lines of the patch below, if you see any value in
it. (not tested, probably not quite right).

       Arnd

diff --git a/net/bridge/br_ioctl.c b/net/bridge/br_ioctl.c
index db4ab2c2ce18..138aa96d73f9 100644
--- a/net/bridge/br_ioctl.c
+++ b/net/bridge/br_ioctl.c
@@ -102,19 +102,9 @@ static int add_del_if(struct net_bridge *br, int
ifindex, int isadd)
        return ret;
 }

-/*
- * 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)
+static int br_dev_read_uargs(unsigned long *args, void __user *argp,
+                            void __user *data)
 {
-       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];

@@ -126,13 +116,29 @@ int br_dev_siocdevprivate(struct net_device
*dev, struct ifreq *rq, void __user
                args[2] = cargs[2];
                args[3] = cargs[3];

-               argp = compat_ptr(args[1]);
+               *argp = compat_ptr(args[1]);
        } else {
                if (copy_from_user(args, data, sizeof(args)))
                        return -EFAULT;

-               argp = (void __user *)args[1];
+               *argp = (void __user *)args[1];
        }
+}
+
+/*
+ * 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)
+{
+       struct net_bridge *br = netdev_priv(dev);
+       struct net_bridge_port *p = NULL;
+       unsigned long args[4];
+       void __user *argp;
+       int ret;
+
+       ret = br_dev_read_uargs(args, &argp, data);

        switch (args[0]) {
        case BRCTL_ADD_IF:
@@ -301,6 +307,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) {
@@ -315,10 +324,13 @@ int br_dev_siocdevprivate(struct net_device
*dev, struct ifreq *rq, void __user

 static int old_deviceless(struct net *net, void __user *uarg)
 {
-       unsigned long args[3];
+       unsigned long args[4];
+       void __user *argp;
+       int ret;

-       if (copy_from_user(args, uarg, sizeof(args)))
-               return -EFAULT;
+       ret = br_dev_read_uargs(args, &argp, data);
+       if (ret)
+               return ret;

        switch (args[0]) {
        case BRCTL_GET_VERSION:
@@ -337,7 +349,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(argp, indices,
                                   array_size(args[2], sizeof(int)))
                        ? -EFAULT : args[2];

@@ -353,7 +365,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;

Re: [PATCH net] net: bridge: fix ioctl old_deviceless bridge argument

[Remi Pommarel]

On Wed, Dec 22, 2021 at 10:52:20PM +0100, Arnd Bergmann wrote:
> On Wed, Dec 22, 2021 at 8:13 PM Remi Pommarel <repk@triplefau.lt> wrote:
[...]
> 
> The intention of my broken patch was to make it work for compat mode as I did
> in br_dev_siocdevprivate(), as this is now the only bit that remains broken.
> 
> This could be done along the lines of the patch below, if you see any value in
> it. (not tested, probably not quite right).

Oh ok, because SIOC{S,G}IFBR compat ioctl was painfully done with
old_bridge_ioctl() I didn't think those needed compat. So I adapted and
fixed your patch to get that working.

Here is my test results.

With my initial patch only :
  - 64bit busybox's brctl (working)
    # brctl show
    bridge name     bridge id               STP enabled     interfaces
    br0             8000.000000000000       n

  - CONFIG_COMPAT=y + 32bit busybox's brctl (not working)
    # brctl show
    brctl: SIOCGIFBR: Invalid argument

With both my intial patch and the one below :
  - 64bit busybox's brctl (working)
    # brctl show
    bridge name     bridge id               STP enabled     interfaces
    br0             8000.000000000000       n

  - CONFIG_COMPAT=y + 32bit busybox's brctl (working)
    # brctl show
    bridge name     bridge id               STP enabled     interfaces
    br0             8000.000000000000       n

If you think this has enough value to fix those compatility issues I can
either send the below patch as a V2 replacing my initial one for net
or sending it as a separate patch for net-next. What would you rather
like ?

Thanks

-- 
Remi

diff --git a/net/bridge/br_ioctl.c b/net/bridge/br_ioctl.c
index ca02db813c72..cbb373acf294 100644
--- a/net/bridge/br_ioctl.c
+++ b/net/bridge/br_ioctl.c
@@ -101,37 +101,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:
@@ -300,6 +319,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) {
@@ -312,12 +334,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:
@@ -336,7 +361,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,
 				   args[2]*sizeof(int)) ? -EFAULT : args[2];
 
 		kfree(indices);
@@ -351,7 +376,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 d9d036cad388..f5b107c5cb32 100644
--- a/net/socket.c
+++ b/net/socket.c
@@ -3278,21 +3278,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)
 {
@@ -3304,9 +3289,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:
@@ -3335,6 +3317,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:

Re: [PATCH net] net: bridge: fix ioctl old_deviceless bridge argument

[Arnd Bergmann]

On Thu, Dec 23, 2021 at 12:00 PM Remi Pommarel <repk@triplefau.lt> wrote:
>
> On Wed, Dec 22, 2021 at 10:52:20PM +0100, Arnd Bergmann wrote:
> > On Wed, Dec 22, 2021 at 8:13 PM Remi Pommarel <repk@triplefau.lt> wrote:
> [...]
> >
> > The intention of my broken patch was to make it work for compat mode as I did
> > in br_dev_siocdevprivate(), as this is now the only bit that remains broken.
> >
> > This could be done along the lines of the patch below, if you see any value in
> > it. (not tested, probably not quite right).
>
> Oh ok, because SIOC{S,G}IFBR compat ioctl was painfully done with
> old_bridge_ioctl() I didn't think those needed compat. So I adapted and
> fixed your patch to get that working.

Ok, thanks!

> Here is my test results.
>
> With my initial patch only :
>   - 64bit busybox's brctl (working)
>     # brctl show
>     bridge name     bridge id               STP enabled     interfaces
>     br0             8000.000000000000       n
>
>   - CONFIG_COMPAT=y + 32bit busybox's brctl (not working)
>     # brctl show
>     brctl: SIOCGIFBR: Invalid argument
>
> With both my intial patch and the one below :
>   - 64bit busybox's brctl (working)
>     # brctl show
>     bridge name     bridge id               STP enabled     interfaces
>     br0             8000.000000000000       n
>
>   - CONFIG_COMPAT=y + 32bit busybox's brctl (working)
>     # brctl show
>     bridge name     bridge id               STP enabled     interfaces
>     br0             8000.000000000000       n
>
> If you think this has enough value to fix those compatility issues I can
> either send the below patch as a V2 replacing my initial one for net
> or sending it as a separate patch for net-next. What would you rather
> like ?

If 32-bit busybox still uses those ioctls in moderately recent
versions, then it's probably worth doing this, but that would
be up to the bridge maintainers.

Your patch looks good to me, I see you caught a few mistakes
in my prototype. I would however suggest basing it on top of
your original fix, so that can be applied first and backported
to stable kernels, while the new patch would go on top and
not get backported.

If that works with everyone, please submit those two, and add
these tags to the second patch:

Co-developed-by: Arnd Bergmann <arnd@arndb.de>
Signed-off-by: Arnd Bergmann <arnd@arndb.de>

Re: [PATCH net] net: bridge: fix ioctl old_deviceless bridge argument

[Remi Pommarel]

On Thu, Dec 23, 2021 at 12:38:14PM +0100, Arnd Bergmann wrote:
> On Thu, Dec 23, 2021 at 12:00 PM Remi Pommarel <repk@triplefau.lt> wrote:
> >
> > On Wed, Dec 22, 2021 at 10:52:20PM +0100, Arnd Bergmann wrote:
> > > On Wed, Dec 22, 2021 at 8:13 PM Remi Pommarel <repk@triplefau.lt> wrote:
> > [...]
> > >
> > > The intention of my broken patch was to make it work for compat mode as I did
> > > in br_dev_siocdevprivate(), as this is now the only bit that remains broken.
> > >
> > > This could be done along the lines of the patch below, if you see any value in
> > > it. (not tested, probably not quite right).
> >
> > Oh ok, because SIOC{S,G}IFBR compat ioctl was painfully done with
> > old_bridge_ioctl() I didn't think those needed compat. So I adapted and
> > fixed your patch to get that working.
> 
> Ok, thanks!
> 
> > Here is my test results.
> >
> > With my initial patch only :
> >   - 64bit busybox's brctl (working)
> >     # brctl show
> >     bridge name     bridge id               STP enabled     interfaces
> >     br0             8000.000000000000       n
> >
> >   - CONFIG_COMPAT=y + 32bit busybox's brctl (not working)
> >     # brctl show
> >     brctl: SIOCGIFBR: Invalid argument
> >
> > With both my intial patch and the one below :
> >   - 64bit busybox's brctl (working)
> >     # brctl show
> >     bridge name     bridge id               STP enabled     interfaces
> >     br0             8000.000000000000       n
> >
> >   - CONFIG_COMPAT=y + 32bit busybox's brctl (working)
> >     # brctl show
> >     bridge name     bridge id               STP enabled     interfaces
> >     br0             8000.000000000000       n
> >
> > If you think this has enough value to fix those compatility issues I can
> > either send the below patch as a V2 replacing my initial one for net
> > or sending it as a separate patch for net-next. What would you rather
> > like ?
> 
> If 32-bit busybox still uses those ioctls in moderately recent
> versions, then it's probably worth doing this, but that would
> be up to the bridge maintainers.
> 
> Your patch looks good to me, I see you caught a few mistakes
> in my prototype. I would however suggest basing it on top of
> your original fix, so that can be applied first and backported
> to stable kernels, while the new patch would go on top and
> not get backported.
> 
> If that works with everyone, please submit those two, and add
> these tags to the second patch:
> 
> Co-developed-by: Arnd Bergmann <arnd@arndb.de>
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>

Ok thanks a lot, will send a new patch serie with both patches so
that bridge maintainers could only pick one or both patches.

-- 
Remi

Re: [PATCH net] net: bridge: fix ioctl old_deviceless bridge argument

[Nikolay Aleksandrov]

On 22/12/2021 21:13, Remi Pommarel wrote:
> 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
> 
> Fixes: 561d8352818f ("bridge: use ndo_siocdevprivate")
> Signed-off-by: Remi Pommarel <repk@triplefau.lt>
> ---
>  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];
>  
> 

Acked-by: Nikolay Aleksandrov <nikolay@nvidia.com>

Re: [PATCH net] net: bridge: fix ioctl old_deviceless bridge argument

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net.git (master)
by Jakub Kicinski <kuba@kernel.org>:

On Wed, 22 Dec 2021 20:13:20 +0100 you wrote:
> 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.
> 
> [...]

Here is the summary with links:
  - [net] net: bridge: fix ioctl old_deviceless bridge argument
    https://git.kernel.org/netdev/net/c/d95a56207c07

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html