natsemi.c ioctl fix or mii register access

natsemi.c ioctl fix or mii register access

[glenn_engel]

Hi,

I recently discovered the ioctl implementation in natsemi.c had a few bugs in dealing with the user ioctls to send and receive MII commands (SIOCGMIIPHY and SIOCSMIIPHY).
 
The specific problems noted and fixed:

1. The if_mii macro casts it's return to be (struct mii_ioctl_data *) but in reality it returns a pointer to the user space pointer (struct mii_ioctl_data**).   This looks to be a problem with the mii_macro to me.  I changed this to use the ifr_data macro instead.

2. Since the mii_ioctl_data structure resides in user space, it must be copied into kernel space before access and copied back for read results.  References to the pointer were changed to point to the local copy (data-> changed to mii_data.)

--
Glenn

--- linux-2.6.25.4.orig/drivers/net/natsemi.c	2008-05-15 14:05:37.000000000 -0700
+++ linux-2.6.25.4/drivers/net/natsemi.c	2008-05-15 14:29:28.000000000 -0700
@@ -3043,13 +3043,18 @@
 
 static int netdev_ioctl(struct net_device *dev, struct ifreq *rq, int cmd)
 {
-	struct mii_ioctl_data *data = if_mii(rq);
+	struct mii_ioctl_data *data = (struct mii_ioctl_data*)rq->ifr_data;
 	struct netdev_private *np = netdev_priv(dev);
+	struct mii_ioctl_data mii_data;
+    
+	if (cmd == SIOCGMIIPHY || cmd == SIOCGMIIREG || cmd == SIOCSMIIREG) {
+		copy_from_user(&mii_data, data, sizeof(mii_data));
+	}
 
 	switch(cmd) {
 	case SIOCGMIIPHY:		/* Get address of MII PHY in use. */
 	case SIOCDEVPRIVATE:		/* for binary compat, remove in 2.5 */
-		data->phy_id = np->phy_addr_external;
+		mii_data.phy_id = np->phy_addr_external;
 		/* Fall Through */
 
 	case SIOCGMIIREG:		/* Read MII PHY register. */
@@ -3059,16 +3064,17 @@
 		 * the given mii on the current port.
 		 */
 		if (dev->if_port == PORT_TP) {
-			if ((data->phy_id & 0x1f) == np->phy_addr_external)
-				data->val_out = mdio_read(dev,
-							data->reg_num & 0x1f);
+			if ((mii_data.phy_id & 0x1f) == np->phy_addr_external)
+				mii_data.val_out = mdio_read(dev,
+							mii_data.reg_num & 0x1f);
 			else
-				data->val_out = 0;
+				mii_data.val_out = 0;
 		} else {
-			move_int_phy(dev, data->phy_id & 0x1f);
-			data->val_out = miiport_read(dev, data->phy_id & 0x1f,
-							data->reg_num & 0x1f);
+			move_int_phy(dev, mii_data.phy_id & 0x1f);
+			mii_data.val_out = miiport_read(dev, mii_data.phy_id & 0x1f,
+							mii_data.reg_num & 0x1f);
 		}
+		copy_to_user(data, &mii_data, sizeof(mii_data));
 		return 0;
 
 	case SIOCSMIIREG:		/* Write MII PHY register. */
@@ -3076,21 +3082,21 @@
 		if (!capable(CAP_NET_ADMIN))
 			return -EPERM;
 		if (dev->if_port == PORT_TP) {
-			if ((data->phy_id & 0x1f) == np->phy_addr_external) {
- 				if ((data->reg_num & 0x1f) == MII_ADVERTISE)
-					np->advertising = data->val_in;
-				mdio_write(dev, data->reg_num & 0x1f,
-							data->val_in);
+			if ((mii_data.phy_id & 0x1f) == np->phy_addr_external) {
+ 				if ((mii_data.reg_num & 0x1f) == MII_ADVERTISE)
+					np->advertising = mii_data.val_in;
+				mdio_write(dev, mii_data.reg_num & 0x1f,
+							mii_data.val_in);
 			}
 		} else {
-			if ((data->phy_id & 0x1f) == np->phy_addr_external) {
- 				if ((data->reg_num & 0x1f) == MII_ADVERTISE)
-					np->advertising = data->val_in;
+			if ((mii_data.phy_id & 0x1f) == np->phy_addr_external) {
+ 				if ((mii_data.reg_num & 0x1f) == MII_ADVERTISE)
+					np->advertising = mii_data.val_in;
 			}
-			move_int_phy(dev, data->phy_id & 0x1f);
-			miiport_write(dev, data->phy_id & 0x1f,
-						data->reg_num & 0x1f,
-						data->val_in);
+			move_int_phy(dev, mii_data.phy_id & 0x1f);
+			miiport_write(dev, mii_data.phy_id & 0x1f,
+						mii_data.reg_num & 0x1f,
+						mii_data.val_in);
 		}
 		return 0;
 	default:

Re: natsemi.c ioctl fix or mii register access

[Francois Romieu]

(please shorten your lines below 80 cols)

glenn_engel@agilent.com <glenn_engel@agilent.com> :
[...]
> The specific problems noted and fixed:
> 
> 1. The if_mii macro casts it's return to be (struct mii_ioctl_data *) but
> in reality it returns a pointer to the user space pointer (struct
> mii_ioctl_data**).   This looks to be a problem with the mii_macro to me.
> I changed this to use the ifr_data macro instead.

Which user space application did you read the source of ?

Afaiks the kernel side looks very similar to any usual network device
driver ioctl.

> 2. Since the mii_ioctl_data structure resides in user space, it must be
> copied into kernel space before access and copied back for read results.

Is not it what net/core/dev.c::dev_ioctl does.

-- 
Ueimor

Re: natsemi.c ioctl fix or mii register access

[Jeff Garzik]

glenn_engel@agilent.com wrote:
> Hi,
> 
> I recently discovered the ioctl implementation in natsemi.c had a few bugs in dealing with the user ioctls to send and receive MII commands (SIOCGMIIPHY and SIOCSMIIPHY).
>  
> The specific problems noted and fixed:
> 
> 1. The if_mii macro casts it's return to be (struct mii_ioctl_data *) but in reality it returns a pointer to the user space pointer (struct mii_ioctl_data**).   This looks to be a problem with the mii_macro to me.  I changed this to use the ifr_data macro instead.
> 
> 2. Since the mii_ioctl_data structure resides in user space, it must be copied into kernel space before access and copied back for read results.  References to the pointer were changed to point to the local copy (data-> changed to mii_data.)

This is completely incorrect.  The copying is done for us inside net core.

	Jeff

RE: natsemi.c ioctl fix or mii register access

[glenn_engel]

The core only copies the ifreq struct.  In the case of SIOC[GS]MIIPHY the ifru_data field is a pointer to additional user space information about the ioctl (phy_id, register number).

Thus, an additional copy_from/to_user is required to extract this additional information.  

I suppose this copy could be done in dev.c for these ioctls but it would also need to tweak the ifru_data field to point to local storage before/after the ioctl calls.  Perhaps this was the original intent and was not noticed since ethtool doesn't use these ioctls.  I just assumed the natsemi driver was the odd duck but perhaps none of them work with these ioctls at this point.

--
Glenn

> -----Original Message-----
> From: Jeff Garzik [mailto:jgarzik@pobox.com]
> Sent: Thursday, May 22, 2008 11:05 AM
> To: ENGEL,GLENN (A-Labs,ex1)
> Cc: netdev@vger.kernel.org
> Subject: Re: natsemi.c ioctl fix or mii register access
> 
> glenn_engel@agilent.com wrote:
> > Hi,
> >
> > I recently discovered the ioctl implementation in natsemi.c had a few
> bugs in dealing with the user ioctls to send and receive MII commands
> (SIOCGMIIPHY and SIOCSMIIPHY).
> >
> > The specific problems noted and fixed:
> >
> > 1. The if_mii macro casts it's return to be (struct mii_ioctl_data *)
> but in reality it returns a pointer to the user space pointer (struct
> mii_ioctl_data**).   This looks to be a problem with the mii_macro to
> me.  I changed this to use the ifr_data macro instead.
> >
> > 2. Since the mii_ioctl_data structure resides in user space, it must
> be copied into kernel space before access and copied back for read
> results.  References to the pointer were changed to point to the local
> copy (data-> changed to mii_data.)
> 
> This is completely incorrect.  The copying is done for us inside net
> core.
> 
> 	Jeff
> 
> 
> 

Re: natsemi.c ioctl fix or mii register access

[Francois Romieu]

(please don't top post and quote only the relevant parts)

glenn_engel@agilent.com <glenn_engel@agilent.com> :
[...]
> I suppose this copy could be done in dev.c for these ioctls but it would
> also need to tweak the ifru_data field to point to local storage
> before/after the ioctl calls.  Perhaps this was the original intent and
> was not noticed since ethtool doesn't use these ioctls. I just assumed
> the natsemi driver was the odd duck but perhaps none of them work with
> these ioctls at this point.

The type-casting is a bit confusing at times but these ioctls work rather
well in real life, with mii-tool for instance.

-- 
Ueimor

RE: natsemi.c ioctl fix or mii register access

[glenn_engel]

[...]
> > I suppose this copy could be done in dev.c for these ioctls but it
> would
> > also need to tweak the ifru_data field to point to local storage
> > before/after the ioctl calls.  Perhaps this was the original intent
> and
> > was not noticed since ethtool doesn't use these ioctls. I just
> assumed
> > the natsemi driver was the odd duck but perhaps none of them work
> with
> > these ioctls at this point.
> 
> The type-casting is a bit confusing at times but these ioctls work
> rather
> well in real life, with mii-tool for instance.
> 

OK, I see what you mean.  Even though the ifru_data field is a __user* pointer and nominally 4 bytes long, the mii-tool program is treating it as an 8 byte mii_ioctl_data info structure and ignoring the fact it's a pointer and relying on the fact that other fields in the union are 8 or more bytes long.

Now that I understand the usage I'm wondering if there was a way for me to intuit that the __user* pointer was not really intended to be a user space pointer other than finding the mii-tool source?

Thanks for clearing this up.

--
Glenn