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

[Simon Horman <horms@kernel.org>]

On Tue, Nov 21, 2023 at 02:51:04AM +0000, Yahui Cao wrote:
> From: Lingyu Liu <lingyu.liu@intel.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@intel.com>
> Signed-off-by: Yahui Cao <yahui.cao@intel.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.

> +	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);
> +}

...
_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan