* Re: [PATCH iproute2 v3] ipnetns: use-after-free problem in get_netnsid_from_name func
2019-05-04 7:26 ` [PATCH iproute2 v3] " Zhiqiang Liu
@ 2019-05-04 15:08 ` David Ahern
2019-05-05 1:15 ` Zhiqiang Liu
2019-05-05 1:59 ` [PATCH iproute2 v4] " Zhiqiang Liu
2019-05-06 15:42 ` [PATCH iproute2 v3] " Stephen Hemminger
2 siblings, 1 reply; 7+ messages in thread
From: David Ahern @ 2019-05-04 15:08 UTC (permalink / raw)
To: Zhiqiang Liu, stephen, liuhangbin, kuznet
Cc: nicolas.dichtel, phil, wangxiaogang (F), Mingfangsen,
Zhoukang (A), kouhuiying, netdev
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:
>
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH iproute2 v3] ipnetns: use-after-free problem in get_netnsid_from_name func
2019-05-04 15:08 ` David Ahern
@ 2019-05-05 1:15 ` Zhiqiang Liu
0 siblings, 0 replies; 7+ messages in thread
From: Zhiqiang Liu @ 2019-05-05 1:15 UTC (permalink / raw)
To: David Ahern, stephen, liuhangbin, kuznet
Cc: nicolas.dichtel, phil, wangxiaogang (F), Mingfangsen,
Zhoukang (A), kouhuiying, netdev
> 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.
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH iproute2 v4] ipnetns: use-after-free problem in get_netnsid_from_name func
2019-05-04 7:26 ` [PATCH iproute2 v3] " Zhiqiang Liu
2019-05-04 15:08 ` David Ahern
@ 2019-05-05 1:59 ` Zhiqiang Liu
2019-05-06 15:42 ` [PATCH iproute2 v3] " Stephen Hemminger
2 siblings, 0 replies; 7+ messages in thread
From: Zhiqiang Liu @ 2019-05-05 1:59 UTC (permalink / raw)
To: stephen, liuhangbin, kuznet
Cc: nicolas.dichtel, phil, wangxiaogang (F), Mingfangsen,
Zhoukang (A), kouhuiying, netdev
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>
---
v3->v4: optimize code suggested by David Ahern
v2->v3: add Cc:netdev@vger.kernel.org suggested by Phil Sutter
v1->v2: correct commit log
ip/ipnetns.c | 13 ++++++-------
1 file changed, 6 insertions(+), 7 deletions(-)
diff --git a/ip/ipnetns.c b/ip/ipnetns.c
index 430d884..52aefac 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();
@@ -124,23 +124,22 @@ int get_netnsid_from_name(const char *name)
/* Validate message and parse attributes */
if (answer->nlmsg_type == NLMSG_ERROR)
- goto err_out;
+ goto out;
rthdr = NLMSG_DATA(answer);
len = answer->nlmsg_len - NLMSG_SPACE(sizeof(*rthdr));
if (len < 0)
- goto err_out;
+ goto out;
parse_rtattr(tb, NETNSA_MAX, NETNS_RTA(rthdr), len);
if (tb[NETNSA_NSID]) {
- free(answer);
- return rta_getattr_u32(tb[NETNSA_NSID]);
+ ret = rta_getattr_u32(tb[NETNSA_NSID]);
}
-err_out:
+out:
free(answer);
- return -1;
+ return ret;
}
struct nsid_cache {
--
1.8.3.1
^ permalink raw reply related [flat|nested] 7+ messages in thread* Re: [PATCH iproute2 v3] ipnetns: use-after-free problem in get_netnsid_from_name func
2019-05-04 7:26 ` [PATCH iproute2 v3] " Zhiqiang Liu
2019-05-04 15:08 ` David Ahern
2019-05-05 1:59 ` [PATCH iproute2 v4] " Zhiqiang Liu
@ 2019-05-06 15:42 ` Stephen Hemminger
2019-05-06 15:50 ` Zhiqiang Liu
2 siblings, 1 reply; 7+ messages in thread
From: Stephen Hemminger @ 2019-05-06 15:42 UTC (permalink / raw)
To: Zhiqiang Liu
Cc: liuhangbin, kuznet, nicolas.dichtel, phil, wangxiaogang (F),
Mingfangsen, Zhoukang (A), kouhuiying, netdev
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.
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH iproute2 v3] ipnetns: use-after-free problem in get_netnsid_from_name func
2019-05-06 15:42 ` [PATCH iproute2 v3] " Stephen Hemminger
@ 2019-05-06 15:50 ` Zhiqiang Liu
0 siblings, 0 replies; 7+ messages in thread
From: Zhiqiang Liu @ 2019-05-06 15:50 UTC (permalink / raw)
To: Stephen Hemminger
Cc: liuhangbin, kuznet, nicolas.dichtel, phil, wangxiaogang (F),
Mingfangsen, Zhoukang (A), kouhuiying, netdev
> 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.
>
> .
>
^ permalink raw reply [flat|nested] 7+ messages in thread