netdev - Re: [PATCH iwl-next v4 05/12] ice: Log virtual channel messages in PF

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, 1 Dec 2023 16:27:49 +0800
From: "Cao, Yahui" <yahui.cao@...el.com>
To: Simon Horman <horms@...nel.org>
CC: <intel-wired-lan@...ts.osuosl.org>, <kvm@...r.kernel.org>,
	<netdev@...r.kernel.org>, <lingyu.liu@...el.com>, <kevin.tian@...el.com>,
	<madhu.chittim@...el.com>, <sridhar.samudrala@...el.com>,
	<alex.williamson@...hat.com>, <jgg@...dia.com>, <yishaih@...dia.com>,
	<shameerali.kolothum.thodi@...wei.com>, <brett.creeley@....com>,
	<davem@...emloft.net>, <edumazet@...gle.com>, <kuba@...nel.org>,
	<pabeni@...hat.com>
Subject: Re: [PATCH iwl-next v4 05/12] ice: Log virtual channel messages in PF



On 11/30/2023 1:12 AM, Simon Horman wrote:
> On Tue, Nov 21, 2023 at 02:51:04AM +0000, Yahui Cao wrote:
>> From: Lingyu Liu <lingyu.liu@...el.com>
>>
>> Save the virtual channel messages sent by VF on the source side during
>> runtime. The logged virtchnl messages will be transferred and loaded
>> into the device on the destination side during the device resume stage.
>>
>> For the feature which can not be migrated yet, it must be disabled or
>> blocked to prevent from being abused by VF. Otherwise, it may introduce
>> functional and security issue. Mask unsupported VF capability flags in
>> the VF-PF negotiaion stage.
>>
>> Signed-off-by: Lingyu Liu <lingyu.liu@...el.com>
>> Signed-off-by: Yahui Cao <yahui.cao@...el.com>
> 
> Hi Lingyu Liu and Yahui Cao,
> 
> some minor feedback from my side.
> 
> ...
> 
>> diff --git a/drivers/net/ethernet/intel/ice/ice_migration.c b/drivers/net/ethernet/intel/ice/ice_migration.c
> 
> ...
> 
>> +/**
>> + * ice_migration_log_vf_msg - Log request message from VF
>> + * @vf: pointer to the VF structure
>> + * @event: pointer to the AQ event
>> + *
>> + * Log VF message for later device state loading during live migration
>> + *
>> + * Return 0 for success, negative for error
>> + */
>> +int ice_migration_log_vf_msg(struct ice_vf *vf,
>> +			     struct ice_rq_event_info *event)
>> +{
>> +	struct ice_migration_virtchnl_msg_listnode *msg_listnode;
>> +	u32 v_opcode = le32_to_cpu(event->desc.cookie_high);
>> +	struct device *dev = ice_pf_to_dev(vf->pf);
>> +	u16 msglen = event->msg_len;
>> +	u8 *msg = event->msg_buf;
>> +
>> +	if (!ice_migration_is_loggable_msg(v_opcode))
>> +		return 0;
>> +
>> +	if (vf->virtchnl_msg_num >= VIRTCHNL_MSG_MAX) {
>> +		dev_warn(dev, "VF %d has maximum number virtual channel commands\n",
>> +			 vf->vf_id);
>> +		return -ENOMEM;
>> +	}
>> +
>> +	msg_listnode = (struct ice_migration_virtchnl_msg_listnode *)
>> +			kzalloc(struct_size(msg_listnode,
>> +					    msg_slot.msg_buffer,
>> +					    msglen),
>> +				GFP_KERNEL);
> 
> nit: there is no need to cast the void * pointer returned by kzalloc().
> 
> Flagged by Coccinelle.

Sure. Will fix in next version.

> 
>> +	if (!msg_listnode) {
>> +		dev_err(dev, "VF %d failed to allocate memory for msg listnode\n",
>> +			vf->vf_id);
>> +		return -ENOMEM;
>> +	}
>> +	dev_dbg(dev, "VF %d save virtual channel command, op code: %d, len: %d\n",
>> +		vf->vf_id, v_opcode, msglen);
>> +	msg_listnode->msg_slot.opcode = v_opcode;
>> +	msg_listnode->msg_slot.msg_len = msglen;
>> +	memcpy(msg_listnode->msg_slot.msg_buffer, msg, msglen);
>> +	list_add_tail(&msg_listnode->node, &vf->virtchnl_msg_list);
>> +	vf->virtchnl_msg_num++;
>> +	vf->virtchnl_msg_size += struct_size(&msg_listnode->msg_slot,
>> +					     msg_buffer,
>> +					     msglen);
>> +	return 0;
>> +}
>> +
>> +/**
>> + * ice_migration_unlog_vf_msg - revert logged message
>> + * @vf: pointer to the VF structure
>> + * @v_opcode: virtchnl message operation code
>> + *
>> + * Remove the last virtual channel message logged before.
>> + */
>> +void ice_migration_unlog_vf_msg(struct ice_vf *vf, u32 v_opcode)
>> +{
>> +	struct ice_migration_virtchnl_msg_listnode *msg_listnode;
>> +
>> +	if (!ice_migration_is_loggable_msg(v_opcode))
>> +		return;
>> +
>> +	if (WARN_ON_ONCE(list_empty(&vf->virtchnl_msg_list)))
>> +		return;
>> +
>> +	msg_listnode =
>> +		list_last_entry(&vf->virtchnl_msg_list,
>> +				struct ice_migration_virtchnl_msg_listnode,
>> +				node);
>> +	if (WARN_ON_ONCE(msg_listnode->msg_slot.opcode != v_opcode))
>> +		return;
>> +
>> +	list_del(&msg_listnode->node);
>> +	kfree(msg_listnode);
> 
> msg_listnode is freed on the line above,
> but dereferenced in the usage of struct_size() below.
> 
> As flagged by Smatch and Coccinelle.
>  >> +	vf->virtchnl_msg_num--;
>> +	vf->virtchnl_msg_size -= struct_size(&msg_listnode->msg_slot,
>> +					     msg_buffer,
>> +					     msg_listnode->msg_slot.msg_len);
>> +}
> 
> ...
> 

Good catch :) Will fix in next version

Thanks.
Yahui.