linux-kernel - Re: [PATCH net v2] ice: Fix incorrect locking in ice_vc_process_vf

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:   Fri, 8 Apr 2022 09:01:05 -0700
From:   Tony Nguyen <anthony.l.nguyen@...el.com>
To:     Alexander Lobakin <alexandr.lobakin@...el.com>
CC:     Ivan Vecera <ivecera@...hat.com>, <netdev@...r.kernel.org>,
        <poros@...hat.com>, <mschmidt@...hat.com>,
        <jacob.e.keller@...el.com>,
        "Jesse Brandeburg" <jesse.brandeburg@...el.com>,
        "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>,
        Brett Creeley <brett@...sando.io>,
        "moderated list:INTEL ETHERNET DRIVERS" 
        <intel-wired-lan@...ts.osuosl.org>,
        open list <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH net v2] ice: Fix incorrect locking in
 ice_vc_process_vf_msg()


On 4/8/2022 6:47 AM, Alexander Lobakin wrote:
> From: Ivan Vecera <ivecera@...hat.com>
> Date: Fri,  1 Apr 2022 12:40:52 +0200
>
>> Usage of mutex_trylock() in ice_vc_process_vf_msg() is incorrect
>> because message sent from VF is ignored and never processed.
>>
>> Use mutex_lock() instead to fix the issue. It is safe because this
>> mutex is used to prevent races between VF related NDOs and
>> handlers processing request messages from VF and these handlers
>> are running in ice_service_task() context. Additionally move this
>> mutex lock prior ice_vc_is_opcode_allowed() call to avoid potential
>> races during allowlist acccess.
>>
>> Fixes: e6ba5273d4ed ("ice: Fix race conditions between virtchnl handling and VF ndo ops")
>> Signed-off-by: Ivan Vecera <ivecera@...hat.com>
> Hey Tony,
>
> I guess you missed this one due to being on a vacation previously.
> It's been previously reviewed IIRC, could you take it into
> net-queue?

I remember applying this but I don't see it on the tree so I must be 
mistaken. :( I'll get it applied, thanks for catching.

-Tony

>> ---
>>   drivers/net/ethernet/intel/ice/ice_virtchnl.c | 21 +++++++------------
>>   1 file changed, 7 insertions(+), 14 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/intel/ice/ice_virtchnl.c b/drivers/net/ethernet/intel/ice/ice_virtchnl.c
>> index 3f1a63815bac..a465f3743ffc 100644
>> --- a/drivers/net/ethernet/intel/ice/ice_virtchnl.c
>> +++ b/drivers/net/ethernet/intel/ice/ice_virtchnl.c
>> @@ -3642,14 +3642,6 @@ void ice_vc_process_vf_msg(struct ice_pf *pf, struct ice_rq_event_info *event)
>>   			err = -EINVAL;
>>   	}
>>   
>> -	if (!ice_vc_is_opcode_allowed(vf, v_opcode)) {
>> -		ice_vc_send_msg_to_vf(vf, v_opcode,
>> -				      VIRTCHNL_STATUS_ERR_NOT_SUPPORTED, NULL,
>> -				      0);
>> -		ice_put_vf(vf);
>> -		return;
>> -	}
>> -
>>   error_handler:
>>   	if (err) {
>>   		ice_vc_send_msg_to_vf(vf, v_opcode, VIRTCHNL_STATUS_ERR_PARAM,
>> @@ -3660,12 +3652,13 @@ void ice_vc_process_vf_msg(struct ice_pf *pf, struct ice_rq_event_info *event)
>>   		return;
>>   	}
>>   
>> -	/* VF is being configured in another context that triggers a VFR, so no
>> -	 * need to process this message
>> -	 */
>> -	if (!mutex_trylock(&vf->cfg_lock)) {
>> -		dev_info(dev, "VF %u is being configured in another context that will trigger a VFR, so there is no need to handle this message\n",
>> -			 vf->vf_id);
>> +	mutex_lock(&vf->cfg_lock);
>> +
>> +	if (!ice_vc_is_opcode_allowed(vf, v_opcode)) {
>> +		ice_vc_send_msg_to_vf(vf, v_opcode,
>> +				      VIRTCHNL_STATUS_ERR_NOT_SUPPORTED, NULL,
>> +				      0);
>> +		mutex_unlock(&vf->cfg_lock);
>>   		ice_put_vf(vf);
>>   		return;
>>   	}
>> -- 
>> 2.35.1
> Thanks,
> Al