Re: [PATCH v2] xfrm: correctly check policy index in verify_newpolicy_info

[YueHaibing <yuehaibing@huawei.com>]

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,
>