[PATCH] ipx: call ipxitf_put() in ioctl error path

Subsystems: networking [general], the rest

STALE3382d

3 messages, 3 authors, 2017-05-02 · open the first message on its own page

[PATCH] ipx: call ipxitf_put() in ioctl error path

From: Dan Carpenter <hidden>
Date: 2017-05-02 10:59:13

We should call ipxitf_put() if the copy_to_user() fails.

Reported-by: 李强 <redacted>
Signed-off-by: Dan Carpenter <redacted>
diff --git a/net/ipx/af_ipx.c b/net/ipx/af_ipx.c
index 8a9219ff2e77..fa31ef29e3fa 100644
--- a/net/ipx/af_ipx.c
+++ b/net/ipx/af_ipx.c
@@ -1168,11 +1168,10 @@ static int ipxitf_ioctl(unsigned int cmd, void __user *arg)
 		sipx->sipx_network	= ipxif->if_netnum;
 		memcpy(sipx->sipx_node, ipxif->if_node,
 			sizeof(sipx->sipx_node));
-		rc = -EFAULT;
+		rc = 0;
 		if (copy_to_user(arg, &ifr, sizeof(ifr)))
-			break;
+			rc = -EFAULT;
 		ipxitf_put(ipxif);
-		rc = 0;
 		break;
 	}
 	case SIOCAIPXITFCRT:

Re: [PATCH] ipx: call ipxitf_put() in ioctl error path

From: David Miller <davem@davemloft.net>
Date: 2017-05-02 19:35:13

From: Dan Carpenter <redacted>
Date: Tue, 2 May 2017 13:58:53 +0300
We should call ipxitf_put() if the copy_to_user() fails.

Reported-by: 李强 <redacted>
Signed-off-by: Dan Carpenter <redacted>
Applied, thanks Dan.

Re: [PATCH] ipx: call ipxitf_put() in ioctl error path

From: Kees Cook <hidden>
Date: 2017-05-02 23:42:35

On Tue, May 2, 2017 at 3:58 AM, Dan Carpenter [off-list ref] wrote:
quoted hunk
We should call ipxitf_put() if the copy_to_user() fails.

Reported-by: 李强 <redacted>
Signed-off-by: Dan Carpenter <redacted>
diff --git a/net/ipx/af_ipx.c b/net/ipx/af_ipx.c
index 8a9219ff2e77..fa31ef29e3fa 100644
--- a/net/ipx/af_ipx.c
+++ b/net/ipx/af_ipx.c
@@ -1168,11 +1168,10 @@ static int ipxitf_ioctl(unsigned int cmd, void __user *arg)
                sipx->sipx_network      = ipxif->if_netnum;
                memcpy(sipx->sipx_node, ipxif->if_node,
                        sizeof(sipx->sipx_node));
-               rc = -EFAULT;
+               rc = 0;
                if (copy_to_user(arg, &ifr, sizeof(ifr)))
-                       break;
+                       rc = -EFAULT;
                ipxitf_put(ipxif);
-               rc = 0;
                break;
        }
        case SIOCAIPXITFCRT:
This refcount overflow flaw (and resulting use-after-free) appears to
be reachable from unprivileged userspace, though I think it requires
an interface already be configured, so this is likely not much risk to
most users. Someone more familiar with IPX should double-check...

And, of course, I should point out this flaw would have been blocked
entirely by using refcount_t:
https://lkml.org/lkml/2017/3/17/193

And if it didn't require a configured interface, it would be mitigated
with module autoload blocking:
https://lkml.org/lkml/2017/4/19/1088

(Yes, yes, I know both are still being worked on, but this is a good
example to show another case where they'd have been useful.)

-Kees

-- 
Kees Cook
Pixel Security
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help