netdev - Re: [PATCH net 2/4] bonding: fix null pointer deref in bond_ipsec_offload

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <a8ebc617-dc20-4803-9332-246d54ccf8d8@blackwall.org>
Date: Mon, 19 Aug 2024 10:25:37 +0300
From: Nikolay Aleksandrov <razor@...ckwall.org>
To: Hangbin Liu <liuhangbin@...il.com>
Cc: netdev@...r.kernel.org, Taehee Yoo <ap420073@...il.com>,
 davem@...emloft.net, jv@...sburgh.net, andy@...yhouse.net,
 edumazet@...gle.com, kuba@...nel.org, pabeni@...hat.com, jarod@...hat.com
Subject: Re: [PATCH net 2/4] bonding: fix null pointer deref in
 bond_ipsec_offload_ok

On 19/08/2024 05:53, Hangbin Liu wrote:
> On Fri, Aug 16, 2024 at 02:48:11PM +0300, Nikolay Aleksandrov wrote:
>> We must check if there is an active slave before dereferencing the pointer.
>>
>> Fixes: 18cb261afd7b ("bonding: support hardware encryption offload to slaves")
>> Signed-off-by: Nikolay Aleksandrov <razor@...ckwall.org>
>> ---
>>  drivers/net/bonding/bond_main.c | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
>> index 85b5868deeea..65ddb71eebcd 100644
>> --- a/drivers/net/bonding/bond_main.c
>> +++ b/drivers/net/bonding/bond_main.c
>> @@ -604,6 +604,8 @@ static bool bond_ipsec_offload_ok(struct sk_buff *skb, struct xfrm_state *xs)
>>  	bond = netdev_priv(bond_dev);
>>  	rcu_read_lock();
>>  	curr_active = rcu_dereference(bond->curr_active_slave);
>> +	if (!curr_active)
>> +		goto out;
>>  	real_dev = curr_active->dev;
>>  
>>  	if (BOND_MODE(bond) != BOND_MODE_ACTIVEBACKUP)
>> -- 
>> 2.44.0
>>
> 
> BTW, the bond_ipsec_offload_ok() only checks !xs->xso.real_dev, should we also
> add WARN_ON(xs->xso.real_dev != slave->dev) checking?
> 
> Thanks
> Hangbin

We could, but not a warn_on() because I bet it can be easily triggered
by changing the active slave in parallel. real_dev is read without a
lock here so we cannot guarantee a sane state if policies are changed
under us. I think the callback should handle it by checking that the
new device doesn't have the policy setup yet, because the case happens
when an active slave changes which means policies are about to be
installed on the new one.

Cheers,
 Nik