Re: [PATCH ipsec] xfrm: prevent ipcomp scratch buffer race condition

[Fan Du <fan.du@windriver.com>]

On 2013年10月15日 16:33, Steffen Klassert wrote:
> On Mon, Oct 14, 2013 at 06:03:34PM +0200, Michal Kubecek wrote:
>> In ipcomp_compress(), sortirq is enabled too early, allowing the
>> per-cpu scratch buffer to be rewritten by ipcomp_decompress()
>> (called on the same CPU in softirq context) between populating
>> the buffer and copying the compressed data to the skb.
>>
>> Add similar protection into ipcomp_decompress() as it can be
>> called from process context as well (even if such scenario seems
>> a bit artificial).
>>
>> Signed-off-by: Michal Kubecek<mkubecek@suse.cz>
>> ---
>>   net/xfrm/xfrm_ipcomp.c | 8 ++++++--
>>   1 file changed, 6 insertions(+), 2 deletions(-)
>>
>> diff --git a/net/xfrm/xfrm_ipcomp.c b/net/xfrm/xfrm_ipcomp.c
>> index 2906d52..96946fb 100644
>> --- a/net/xfrm/xfrm_ipcomp.c
>> +++ b/net/xfrm/xfrm_ipcomp.c
>> @@ -48,9 +48,11 @@ static int ipcomp_decompress(struct xfrm_state *x, struct sk_buff *skb)
>>   	const int cpu = get_cpu();
>>   	u8 *scratch = *per_cpu_ptr(ipcomp_scratches, cpu);
>>   	struct crypto_comp *tfm = *per_cpu_ptr(ipcd->tfms, cpu);
>> -	int err = crypto_comp_decompress(tfm, start, plen, scratch,&dlen);
>> +	int err;
>>   	int len;
>>
>> +	local_bh_disable();
>
> Maybe we could disable the BHs before we fetch the percpu pointers.
> Then we can use smp_processor_id() to get the cpu. With that we
> could get rid of a (now useless) preempt_disable()/preempt_enable()
> pair. Same could be done in ipcomp_compress().

Is it possible that two tasks race scratch buffer when both of them trying to compress data
without preempt disabled? for example, when task A working on compression, then task B
with higher priority preempts task A, and try to touch scratch buffer, which leaves stale
data for task A after then.

I think we needs preempt disabled for such case, otherwise I overlook codes in somewhere else.

> Looks ok otherwise. Thanks!
>
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

-- 
浮沉随浪只记今朝笑

--fan