Messages in this thread Patch in this message | | | Subject | Re: [PATCH v2] xfrm: correctly check policy index in verify_newpolicy_info | From | YueHaibing <> | Date | Wed, 27 Feb 2019 11:17:47 +0800 |
| |
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, >
| |