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 ↗ jump to 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