diff mbox series

[v4,2/4] net: socket: rework SIOC?IFMAP ioctls

Message ID 20201124151828.169152-3-arnd@kernel.org
State Superseded
Headers show
Series remove compat_alloc_user_space() | expand

Commit Message

Arnd Bergmann Nov. 24, 2020, 3:18 p.m. UTC 
From: Arnd Bergmann <arnd@arndb.de>

SIOCGIFMAP and SIOCSIFMAP currently require compat_alloc_user_space()
and copy_in_user() for compat mode.

Move the compat handling into the location where the structures are
actually used, to avoid using those interfaces and get a clearer
implementation.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
changes in v3:
 - complete rewrite

changes in v2:
 - fix building with CONFIG_COMPAT disabled (0day bot)
 - split up dev_ifmap() into more readable helpers (hch)
 - move rcu_read_unlock() for readability (hch)
---
 include/linux/compat.h | 18 ++++++------
 net/core/dev_ioctl.c   | 64 +++++++++++++++++++++++++++++++++---------
 net/socket.c           | 39 ++-----------------------
 3 files changed, 62 insertions(+), 59 deletions(-)

Comments

David Laight Nov. 24, 2020, 4:13 p.m. UTC | #1 
From: Arnd Bergmann
> Sent: 24 November 2020 15:18
> 
> SIOCGIFMAP and SIOCSIFMAP currently require compat_alloc_user_space()
> and copy_in_user() for compat mode.
> 
> Move the compat handling into the location where the structures are
> actually used, to avoid using those interfaces and get a clearer
> implementation.
> 
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
> changes in v3:
>  - complete rewrite
...
>  include/linux/compat.h | 18 ++++++------
>  net/core/dev_ioctl.c   | 64 +++++++++++++++++++++++++++++++++---------
>  net/socket.c           | 39 ++-----------------------
>  3 files changed, 62 insertions(+), 59 deletions(-)
> 
> diff --git a/include/linux/compat.h b/include/linux/compat.h
> index 08dbd34bb7a5..47496c5eb5eb 100644
> --- a/include/linux/compat.h
> +++ b/include/linux/compat.h
> @@ -96,6 +96,15 @@ struct compat_iovec {
>  	compat_size_t	iov_len;
>  };
> 
> +struct compat_ifmap {
> +	compat_ulong_t mem_start;
> +	compat_ulong_t mem_end;
> +	unsigned short base_addr;
> +	unsigned char irq;
> +	unsigned char dma;
> +	unsigned char port;
> +};

Isn't the only difference the number of pad bytes at the end?
If you don't copy these (in or out) then the compat version
isn't special at all.
Not copying the pad in or out would ensure you don't leak
kernel stack to userspace.
OTOH you may want to write the padding zero.

So a CT_ASSERT(offsetof (struct ifmap, port) == offsetof (struct compat_ifmap, port))
would suffice.

Maybe a CT_ASSERT_EQ_OFFSET(struct ifmap, struct compat_ifmap, port);
Would make the code easier to read.
Although you might want the version that adds an offset

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
Arnd Bergmann Nov. 24, 2020, 7:05 p.m. UTC | #2 
On Tue, Nov 24, 2020 at 5:13 PM David Laight <David.Laight@aculab.com> wrote:
>
> From: Arnd Bergmann
> > Sent: 24 November 2020 15:18
> >
> > SIOCGIFMAP and SIOCSIFMAP currently require compat_alloc_user_space()
> > and copy_in_user() for compat mode.
> >
> > Move the compat handling into the location where the structures are
> > actually used, to avoid using those interfaces and get a clearer
> > implementation.
> >
> > Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> > ---
> > changes in v3:
> >  - complete rewrite
> ...
> >  include/linux/compat.h | 18 ++++++------
> >  net/core/dev_ioctl.c   | 64 +++++++++++++++++++++++++++++++++---------
> >  net/socket.c           | 39 ++-----------------------
> >  3 files changed, 62 insertions(+), 59 deletions(-)
> >
> > diff --git a/include/linux/compat.h b/include/linux/compat.h
> > index 08dbd34bb7a5..47496c5eb5eb 100644
> > --- a/include/linux/compat.h
> > +++ b/include/linux/compat.h
> > @@ -96,6 +96,15 @@ struct compat_iovec {
> >       compat_size_t   iov_len;
> >  };
> >
> > +struct compat_ifmap {
> > +     compat_ulong_t mem_start;
> > +     compat_ulong_t mem_end;
> > +     unsigned short base_addr;
> > +     unsigned char irq;
> > +     unsigned char dma;
> > +     unsigned char port;
> > +};
>
> Isn't the only difference the number of pad bytes at the end?

No, the main difference is in the first two fields, which are
'unsigned long' and therefore different. The three-byte padding
is in fact the same on all architectures (including x86) that
have a compat mode, though it might be different on
m68k and arm-oabi, which have slightly special struct
alignment rules.

It could be done with two assignments and a memcpy, but
I like the individual assignments better here.

      Arnd
David Laight Nov. 24, 2020, 10:12 p.m. UTC | #3 
From: Arnd Bergmann
> Sent: 24 November 2020 19:06
> 
> On Tue, Nov 24, 2020 at 5:13 PM David Laight <David.Laight@aculab.com> wrote:
> >
> > From: Arnd Bergmann
> > > Sent: 24 November 2020 15:18
> > >
> > > SIOCGIFMAP and SIOCSIFMAP currently require compat_alloc_user_space()
> > > and copy_in_user() for compat mode.
> > >
> > > Move the compat handling into the location where the structures are
> > > actually used, to avoid using those interfaces and get a clearer
> > > implementation.
> > >
> > > Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> > > ---
> > > changes in v3:
> > >  - complete rewrite
> > ...
> > >  include/linux/compat.h | 18 ++++++------
> > >  net/core/dev_ioctl.c   | 64 +++++++++++++++++++++++++++++++++---------
> > >  net/socket.c           | 39 ++-----------------------
> > >  3 files changed, 62 insertions(+), 59 deletions(-)
> > >
> > > diff --git a/include/linux/compat.h b/include/linux/compat.h
> > > index 08dbd34bb7a5..47496c5eb5eb 100644
> > > --- a/include/linux/compat.h
> > > +++ b/include/linux/compat.h
> > > @@ -96,6 +96,15 @@ struct compat_iovec {
> > >       compat_size_t   iov_len;
> > >  };
> > >
> > > +struct compat_ifmap {
> > > +     compat_ulong_t mem_start;
> > > +     compat_ulong_t mem_end;
> > > +     unsigned short base_addr;
> > > +     unsigned char irq;
> > > +     unsigned char dma;
> > > +     unsigned char port;
> > > +};
> >
> > Isn't the only difference the number of pad bytes at the end?
> 
> No, the main difference is in the first two fields, which are
> 'unsigned long' and therefore different. The three-byte padding
> is in fact the same on all architectures (including x86) that
> have a compat mode, though it might be different on
> m68k and arm-oabi, which have slightly special struct
> alignment rules.
> 
> It could be done with two assignments and a memcpy, but
> I like the individual assignments better here.

Gah my brain hurts today.
I was just thinking of the alignment and padding, not the sizes.

You could read the compat structure to 'mem_end' and
then move the first two fields forward.
But, I guess, this structure doesn't have many fields.

What you really need for these copies is COBOL's 'move corresponding'.
(Which wasn't implemented by the only COBOL compiler I've used.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
diff mbox series

Patch

diff --git a/include/linux/compat.h b/include/linux/compat.h
index 08dbd34bb7a5..47496c5eb5eb 100644
--- a/include/linux/compat.h
+++ b/include/linux/compat.h
@@ -96,6 +96,15 @@  struct compat_iovec {
 	compat_size_t	iov_len;
 };
 
+struct compat_ifmap {
+	compat_ulong_t mem_start;
+	compat_ulong_t mem_end;
+	unsigned short base_addr;
+	unsigned char irq;
+	unsigned char dma;
+	unsigned char port;
+};
+
 #ifdef CONFIG_COMPAT
 
 #ifndef compat_user_stack_pointer
@@ -314,15 +323,6 @@  typedef struct compat_sigevent {
 	} _sigev_un;
 } compat_sigevent_t;
 
-struct compat_ifmap {
-	compat_ulong_t mem_start;
-	compat_ulong_t mem_end;
-	unsigned short base_addr;
-	unsigned char irq;
-	unsigned char dma;
-	unsigned char port;
-};
-
 struct compat_if_settings {
 	unsigned int type;	/* Type of physical device or protocol */
 	unsigned int size;	/* Size of the data allocated by the caller */
diff --git a/net/core/dev_ioctl.c b/net/core/dev_ioctl.c
index db8a0ff86f36..de3df6fe65fe 100644
--- a/net/core/dev_ioctl.c
+++ b/net/core/dev_ioctl.c
@@ -98,6 +98,55 @@  int dev_ifconf(struct net *net, struct ifconf *ifc, int size)
 	return 0;
 }
 
+static int dev_getifmap(struct net_device *dev, struct ifreq *ifr)
+{
+	struct ifmap *ifmap = &ifr->ifr_map;
+	struct compat_ifmap *cifmap = (struct compat_ifmap *)&ifr->ifr_map;
+
+	if (in_compat_syscall()) {
+		cifmap->mem_start = dev->mem_start;
+		cifmap->mem_end   = dev->mem_end;
+		cifmap->base_addr = dev->base_addr;
+		cifmap->irq       = dev->irq;
+		cifmap->dma       = dev->dma;
+		cifmap->port      = dev->if_port;
+
+		return 0;
+	}
+
+	ifmap->mem_start  = dev->mem_start;
+	ifmap->mem_end    = dev->mem_end;
+	ifmap->base_addr  = dev->base_addr;
+	ifmap->irq        = dev->irq;
+	ifmap->dma        = dev->dma;
+	ifmap->port       = dev->if_port;
+
+	return 0;
+}
+
+static int dev_setifmap(struct net_device *dev, struct ifreq *ifr)
+{
+	struct compat_ifmap *cifmap = (struct compat_ifmap *)&ifr->ifr_map;
+
+	if (!dev->netdev_ops->ndo_set_config)
+		return -EOPNOTSUPP;
+
+	if (in_compat_syscall()) {
+		struct ifmap ifmap = {
+			.mem_start  = cifmap->mem_start,
+			.mem_end    = cifmap->mem_end,
+			.base_addr  = cifmap->base_addr,
+			.irq        = cifmap->irq,
+			.dma        = cifmap->dma,
+			.port       = cifmap->port,
+		};
+
+		return dev->netdev_ops->ndo_set_config(dev, &ifmap);
+	}
+
+	return dev->netdev_ops->ndo_set_config(dev, &ifr->ifr_map);
+}
+
 /*
  *	Perform the SIOCxIFxxx calls, inside rcu_read_lock()
  */
@@ -139,13 +188,7 @@  static int dev_ifsioc_locked(struct net *net, struct ifreq *ifr, unsigned int cm
 		break;
 
 	case SIOCGIFMAP:
-		ifr->ifr_map.mem_start = dev->mem_start;
-		ifr->ifr_map.mem_end   = dev->mem_end;
-		ifr->ifr_map.base_addr = dev->base_addr;
-		ifr->ifr_map.irq       = dev->irq;
-		ifr->ifr_map.dma       = dev->dma;
-		ifr->ifr_map.port      = dev->if_port;
-		return 0;
+		return dev_getifmap(dev, ifr);
 
 	case SIOCGIFINDEX:
 		ifr->ifr_ifindex = dev->ifindex;
@@ -286,12 +329,7 @@  static int dev_ifsioc(struct net *net, struct ifreq *ifr, unsigned int cmd)
 		return 0;
 
 	case SIOCSIFMAP:
-		if (ops->ndo_set_config) {
-			if (!netif_device_present(dev))
-				return -ENODEV;
-			return ops->ndo_set_config(dev, &ifr->ifr_map);
-		}
-		return -EOPNOTSUPP;
+		return dev_setifmap(dev, ifr);
 
 	case SIOCADDMULTI:
 		if (!ops->ndo_set_rx_mode ||
diff --git a/net/socket.c b/net/socket.c
index 60df84a91051..2d32c8576e95 100644
--- a/net/socket.c
+++ b/net/socket.c
@@ -3198,40 +3198,6 @@  static int compat_ifreq_ioctl(struct net *net, struct socket *sock,
 	return err;
 }
 
-static int compat_sioc_ifmap(struct net *net, unsigned int cmd,
-			struct compat_ifreq __user *uifr32)
-{
-	struct ifreq ifr;
-	struct compat_ifmap __user *uifmap32;
-	int err;
-
-	uifmap32 = &uifr32->ifr_ifru.ifru_map;
-	err = copy_from_user(&ifr, uifr32, sizeof(ifr.ifr_name));
-	err |= get_user(ifr.ifr_map.mem_start, &uifmap32->mem_start);
-	err |= get_user(ifr.ifr_map.mem_end, &uifmap32->mem_end);
-	err |= get_user(ifr.ifr_map.base_addr, &uifmap32->base_addr);
-	err |= get_user(ifr.ifr_map.irq, &uifmap32->irq);
-	err |= get_user(ifr.ifr_map.dma, &uifmap32->dma);
-	err |= get_user(ifr.ifr_map.port, &uifmap32->port);
-	if (err)
-		return -EFAULT;
-
-	err = dev_ioctl(net, cmd, &ifr, NULL);
-
-	if (cmd == SIOCGIFMAP && !err) {
-		err = copy_to_user(uifr32, &ifr, sizeof(ifr.ifr_name));
-		err |= put_user(ifr.ifr_map.mem_start, &uifmap32->mem_start);
-		err |= put_user(ifr.ifr_map.mem_end, &uifmap32->mem_end);
-		err |= put_user(ifr.ifr_map.base_addr, &uifmap32->base_addr);
-		err |= put_user(ifr.ifr_map.irq, &uifmap32->irq);
-		err |= put_user(ifr.ifr_map.dma, &uifmap32->dma);
-		err |= put_user(ifr.ifr_map.port, &uifmap32->port);
-		if (err)
-			err = -EFAULT;
-	}
-	return err;
-}
-
 /* Since old style bridge ioctl's endup using SIOCDEVPRIVATE
  * for some operations; this forces use of the newer bridge-utils that
  * use compatible ioctls
@@ -3265,9 +3231,6 @@  static int compat_sock_ioctl_trans(struct file *file, struct socket *sock,
 		return compat_dev_ifconf(net, argp);
 	case SIOCWANDEV:
 		return compat_siocwandev(net, argp);
-	case SIOCGIFMAP:
-	case SIOCSIFMAP:
-		return compat_sioc_ifmap(net, cmd, argp);
 	case SIOCGSTAMP_OLD:
 	case SIOCGSTAMPNS_OLD:
 		if (!sock->ops->gettstamp)
@@ -3297,6 +3260,8 @@  static int compat_sock_ioctl_trans(struct file *file, struct socket *sock,
 
 	case SIOCGIFFLAGS:
 	case SIOCSIFFLAGS:
+	case SIOCGIFMAP:
+	case SIOCSIFMAP:
 	case SIOCGIFMETRIC:
 	case SIOCSIFMETRIC:
 	case SIOCGIFMTU: