| Message ID | 1fca256d-fbce-4da9-471f-14573be4ea21@huawei.com |
|---|---|
| State | Superseded |
| Delegated to: | stephen hemminger |
| Headers | show |
| Series | [iproute2,v3] ipnetns: use-after-free problem in get_netnsid_from_name func | expand |
On 5/4/19 1:26 AM, Zhiqiang Liu wrote: > > diff --git a/ip/ipnetns.c b/ip/ipnetns.c > index 430d884..d72be95 100644 > --- a/ip/ipnetns.c > +++ b/ip/ipnetns.c > @@ -107,7 +107,7 @@ int get_netnsid_from_name(const char *name) > struct nlmsghdr *answer; > struct rtattr *tb[NETNSA_MAX + 1]; > struct rtgenmsg *rthdr; > - int len, fd; > + int len, fd, ret = -1; > > netns_nsid_socket_init(); > > @@ -134,8 +134,9 @@ int get_netnsid_from_name(const char *name) > parse_rtattr(tb, NETNSA_MAX, NETNS_RTA(rthdr), len); > > if (tb[NETNSA_NSID]) { > + ret = rta_getattr_u32(tb[NETNSA_NSID]); > free(answer); > - return rta_getattr_u32(tb[NETNSA_NSID]); > + return ret; set ret here, drop the free, let it proceed down to the existing free and return but now using ret. That way there is 1 exit path and handles the cleanup. > } > > err_out: >
> On 5/4/19 1:26 AM, Zhiqiang Liu wrote: >> >> diff --git a/ip/ipnetns.c b/ip/ipnetns.c >> index 430d884..d72be95 100644 >> --- a/ip/ipnetns.c >> +++ b/ip/ipnetns.c >> @@ -107,7 +107,7 @@ int get_netnsid_from_name(const char *name) >> struct nlmsghdr *answer; >> struct rtattr *tb[NETNSA_MAX + 1]; >> struct rtgenmsg *rthdr; >> - int len, fd; >> + int len, fd, ret = -1; >> >> netns_nsid_socket_init(); >> >> @@ -134,8 +134,9 @@ int get_netnsid_from_name(const char *name) >> parse_rtattr(tb, NETNSA_MAX, NETNS_RTA(rthdr), len); >> >> if (tb[NETNSA_NSID]) { >> + ret = rta_getattr_u32(tb[NETNSA_NSID]); >> free(answer); >> - return rta_getattr_u32(tb[NETNSA_NSID]); >> + return ret; > > set ret here, drop the free, let it proceed down to the existing free > and return but now using ret. That way there is 1 exit path and handles > the cleanup Yes, you are right. I will do that in the v4 patch. That is very nice of you. Thanks a lot.
On Sat, 4 May 2019 15:26:25 +0800 Zhiqiang Liu <liuzhiqiang26@huawei.com> wrote: > From: Zhiqiang Liu <liuzhiqiang26@huawei.com> > > Follow the following steps: > # ip netns add net1 > # export MALLOC_MMAP_THRESHOLD_=0 > # ip netns list > then Segmentation fault (core dumped) will occur. > > In get_netnsid_from_name func, answer is freed before rta_getattr_u32(tb[NETNSA_NSID]), > where tb[] refers to answer`s content. If we set MALLOC_MMAP_THRESHOLD_=0, mmap will > be adoped to malloc memory, which will be freed immediately after calling free func. > So reading tb[NETNSA_NSID] will access the released memory after free(answer). > > Here, we will call get_netnsid_from_name(tb[NETNSA_NSID]) before free(answer). > > Fixes: 86bf43c7c2f ("lib/libnetlink: update rtnl_talk to support malloc buff at run time") > Reported-by: Huiying Kou <kouhuiying@huawei.com> > Signed-off-by: Zhiqiang Liu <liuzhiqiang26@huawei.com> > Acked-by: Phil Sutter <phil@nwl.cc> Applied. You can get better and more detailed checks by running with valgrind. Which is what I did after applying your patch.
> On Sat, 4 May 2019 15:26:25 +0800 > Zhiqiang Liu <liuzhiqiang26@huawei.com> wrote: > >> From: Zhiqiang Liu <liuzhiqiang26@huawei.com> >> >> Follow the following steps: >> # ip netns add net1 >> # export MALLOC_MMAP_THRESHOLD_=0 >> # ip netns list >> then Segmentation fault (core dumped) will occur. >> >> In get_netnsid_from_name func, answer is freed before rta_getattr_u32(tb[NETNSA_NSID]), >> where tb[] refers to answer`s content. If we set MALLOC_MMAP_THRESHOLD_=0, mmap will >> be adoped to malloc memory, which will be freed immediately after calling free func. >> So reading tb[NETNSA_NSID] will access the released memory after free(answer). >> >> Here, we will call get_netnsid_from_name(tb[NETNSA_NSID]) before free(answer). >> >> Fixes: 86bf43c7c2f ("lib/libnetlink: update rtnl_talk to support malloc buff at run time") >> Reported-by: Huiying Kou <kouhuiying@huawei.com> >> Signed-off-by: Zhiqiang Liu <liuzhiqiang26@huawei.com> >> Acked-by: Phil Sutter <phil@nwl.cc> > > Applied. You can get better and more detailed checks by running with > valgrind. Which is what I did after applying your patch. Thank you for your advice. I will learn how to use valgrind, and use it to obtain more detailed checks. > > . >
diff --git a/ip/ipnetns.c b/ip/ipnetns.c index 430d884..d72be95 100644 --- a/ip/ipnetns.c +++ b/ip/ipnetns.c @@ -107,7 +107,7 @@ int get_netnsid_from_name(const char *name) struct nlmsghdr *answer; struct rtattr *tb[NETNSA_MAX + 1]; struct rtgenmsg *rthdr; - int len, fd; + int len, fd, ret = -1; netns_nsid_socket_init(); @@ -134,8 +134,9 @@ int get_netnsid_from_name(const char *name) parse_rtattr(tb, NETNSA_MAX, NETNS_RTA(rthdr), len); if (tb[NETNSA_NSID]) { + ret = rta_getattr_u32(tb[NETNSA_NSID]); free(answer); - return rta_getattr_u32(tb[NETNSA_NSID]); + return ret; } err_out: