linux-hardening - Re: [PATCH net v4 2/2] iavf: Fix out-of-bounds when setting channels on remove

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]

Date:   Wed, 3 May 2023 12:22:00 -0700
From:   "Chittim, Madhu" <madhu.chittim@...el.com>
To:     Leon Romanovsky <leon@...nel.org>,
        Ding Hui <dinghui@...gfor.com.cn>
CC:     <davem@...emloft.net>, <edumazet@...gle.com>, <kuba@...nel.org>,
        <pabeni@...hat.com>, <intel-wired-lan@...ts.osuosl.org>,
        <jesse.brandeburg@...el.com>, <anthony.l.nguyen@...el.com>,
        <keescook@...omium.org>, <grzegorzx.szczurek@...el.com>,
        <mateusz.palczewski@...el.com>, <mitch.a.williams@...el.com>,
        <gregory.v.rose@...el.com>, <jeffrey.t.kirsher@...el.com>,
        <michal.kubiak@...el.com>, <simon.horman@...igine.com>,
        <netdev@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
        <linux-hardening@...r.kernel.org>, <pengdonglin@...gfor.com.cn>,
        <huangcun@...gfor.com.cn>
Subject: Re: [PATCH net v4 2/2] iavf: Fix out-of-bounds when setting channels
 on remove



On 5/3/2023 9:29 AM, Leon Romanovsky wrote:
> On Wed, May 03, 2023 at 10:00:49PM +0800, Ding Hui wrote:
>> On 2023/5/3 4:24 下午, Leon Romanovsky wrote:
>>> On Wed, May 03, 2023 at 11:15:41AM +0800, Ding Hui wrote:
>>
>>>>
>>>> If we detected removing is in processing, we can avoid unnecessary
>>>> waiting and return error faster.
>>>>
>>>> On the other hand in timeout handling, we should keep the original
>>>> num_active_queues and reset num_req_queues to 0.
>>>>
>>>> Fixes: 4e5e6b5d9d13 ("iavf: Fix return of set the new channel count")
>>>> Signed-off-by: Ding Hui <dinghui@...gfor.com.cn>
>>>> Cc: Donglin Peng <pengdonglin@...gfor.com.cn>
>>>> Cc: Huang Cun <huangcun@...gfor.com.cn>
>>>> Reviewed-by: Simon Horman <simon.horman@...igine.com>
>>>> Reviewed-by: Michal Kubiak <michal.kubiak@...el.com>
>>>> ---
>>>> v3 to v4:
>>>>     - nothing changed
>>>>
>>>> v2 to v3:
>>>>     - fix review tag
>>>>
>>>> v1 to v2:
>>>>     - add reproduction script
>>>>
>>>> ---
>>>>    drivers/net/ethernet/intel/iavf/iavf_ethtool.c | 4 +++-
>>>>    1 file changed, 3 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/net/ethernet/intel/iavf/iavf_ethtool.c b/drivers/net/ethernet/intel/iavf/iavf_ethtool.c
>>>> index 6f171d1d85b7..d8a3c0cfedd0 100644
>>>> --- a/drivers/net/ethernet/intel/iavf/iavf_ethtool.c
>>>> +++ b/drivers/net/ethernet/intel/iavf/iavf_ethtool.c
>>>> @@ -1857,13 +1857,15 @@ static int iavf_set_channels(struct net_device *netdev,
>>>>    	/* wait for the reset is done */
>>>>    	for (i = 0; i < IAVF_RESET_WAIT_COMPLETE_COUNT; i++) {
>>>>    		msleep(IAVF_RESET_WAIT_MS);
>>>> +		if (test_bit(__IAVF_IN_REMOVE_TASK, &adapter->crit_section))
>>>> +			return -EOPNOTSUPP;
>>>
>>> This makes no sense without locking as change to __IAVF_IN_REMOVE_TASK
>>> can happen any time.
>>>
>>
>> The state doesn't need to be that precise here, it is optimized only for
>> the fast path. During the lifecycle of the adapter, the __IAVF_IN_REMOVE_TASK
>> state will only be set and not cleared.
>>
>> If we didn't detect the "removing" state, we also can fallback to timeout
>> handling.
>>
>> So I don't think the locking is necessary here, what do the maintainers
>> at Intel think?
> 
> I'm not Intel maintainer, but your change, explanation and the following
> line from your commit message aren't really aligned.
> 
> [ 3510.400799] ==================================================================
> [ 3510.400820] BUG: KASAN: slab-out-of-bounds in iavf_free_all_tx_resources+0x156/0x160 [iavf]
> 
> 

__IAVF_IN_REMOVE_TASK is being set only in iavf_remove() and the above 
change is ok in terms of coming out of setting channels early enough 
while remove is in progress.

Reviewed-by: madhu.chittim@...el.com

>>
>>> Thanks
>>>
>>>>    		if (adapter->flags & IAVF_FLAG_RESET_PENDING)
>>>>    			continue;
>>>>    		break;
>>>>    	}
>>>>    	if (i == IAVF_RESET_WAIT_COMPLETE_COUNT) {
>>>>    		adapter->flags &= ~IAVF_FLAG_REINIT_ITR_NEEDED;
>>>> -		adapter->num_active_queues = num_req;
>>>> +		adapter->num_req_queues = 0;
>>>>    		return -EOPNOTSUPP;
>>>>    	}
>>>> -- 
>>>> 2.17.1
>>>>
>>>>
>>>
>>
>> -- 
>> Thanks,
>> -dinghui
>>
>>