Message ID | 20190225095600.26260-1-yuehaibing@huawei.com |
---|---|
State | Awaiting Upstream |
Delegated to: | David Miller |
Headers | show |
Series | [v2] xfrm: correctly check policy index in verify_newpolicy_info | expand |
On Mon, Feb 25, 2019 at 05:56:00PM +0800, Yue Haibing wrote: > > the check. Then __xfrm_policy_unlink use the index to access array policy_count > whose size is XFRM_POLICY_MAX * 2, triggering out of bounds access. No it doesn't. Even if it did the bug would be in __xfrm_policy_unlink and not here. Your patch makes no sense. Cheers,
On 2019/2/25 21:43, Herbert Xu wrote: > On Mon, Feb 25, 2019 at 05:56:00PM +0800, Yue Haibing wrote: >> >> the check. Then __xfrm_policy_unlink use the index to access array policy_count >> whose size is XFRM_POLICY_MAX * 2, triggering out of bounds access. > > No it doesn't. Even if it did the bug would be in __xfrm_policy_unlink > and not here. > Yes, my fix is wrong. The issue is triggered as this: xfrm_add_policy -->verify_newpolicy_info //here check the index provided by user with XFRM_POLICY_MAX //In my case, the index is 0x6E6BB6, so it pass the check. -->xfrm_policy_construct //copy the user's policy and set xfrm_policy_timer -->xfrm_policy_insert --> __xfrm_policy_link //use the orgin dir, in my case is 2 --> xfrm_gen_index //generate policy index, there is 0x6E6BB6 then xfrm_policy_timer be fired xfrm_policy_timer --> xfrm_policy_id2dir //get dir from policy index & 7, in my case is 6 --> xfrm_policy_delete --> __xfrm_policy_unlink //There access policy_count[dir], it trigger out of range access So maybe the fix is like this: diff --git a/net/xfrm/xfrm_policy.c b/net/xfrm/xfrm_policy.c index 8d1a898..b27eb742 100644 --- a/net/xfrm/xfrm_policy.c +++ b/net/xfrm/xfrm_policy.c @@ -316,6 +316,8 @@ static void xfrm_policy_timer(struct timer_list *t) goto out; dir = xfrm_policy_id2dir(xp->index); + if (dir >= XFRM_POLICY_MAX * 2) + dir = dir & XFRM_POLICY_MAX; if (xp->lft.hard_add_expires_seconds) { time64_t tmo = xp->lft.hard_add_expires_seconds + > Your patch makes no sense. > > Cheers, >
diff --git a/net/xfrm/xfrm_user.c b/net/xfrm/xfrm_user.c index a131f9f..60adacf 100644 --- a/net/xfrm/xfrm_user.c +++ b/net/xfrm/xfrm_user.c @@ -1424,7 +1424,8 @@ static int verify_newpolicy_info(struct xfrm_userpolicy_info *p) ret = verify_policy_dir(p->dir); if (ret) return ret; - if (p->index && ((p->index & XFRM_POLICY_MAX) != p->dir)) + if (p->index && ((p->index >= XFRM_POLICY_MAX * 2) || + (p->index & XFRM_POLICY_MAX) != p->dir)) return -EINVAL; return 0;