Re: [PATCH iwl-next 1/2] idpf: add direct access to discipline the main timer

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] idpf: NULL cmd_sync dereference from overwrite in idpf_ptp_get_caps()
--

commit 89cc323cd92189e1e3d08fc89975837933f93c9d
Author: Przemyslaw Korba <przemyslaw.korba@intel.com>

idpf: add direct access to discipline the main timer

This commit adds support for direct PCI BAR register access to set and adjust
the PTP device clock, alongside validating mapped register offsets and
reconciling dependent PTP feature access modes.

> diff --git a/drivers/net/ethernet/intel/idpf/idpf_virtchnl_ptp.c b/drivers/net/ethernet/intel/idpf/idpf_virtchnl_ptp.c
> index 1422b30d09a31..427b15281ad0d 100644
> --- a/drivers/net/ethernet/intel/idpf/idpf_virtchnl_ptp.c
> +++ b/drivers/net/ethernet/intel/idpf/idpf_virtchnl_ptp.c
[ ... ]
> @@ -90,6 +117,19 @@ int idpf_ptp_get_caps(struct idpf_adapter *adapter)
>  	ptp->dev_clk_regs.cmd_sync =
>  		libie_pci_get_mmio_addr(mmio, temp_offset);
>  
> +	/* Direct access is only usable when every register was mapped. */
> +	void __iomem * const clk_regs[] = {
> +		ptp->dev_clk_regs.dev_clk_ns_l,
> +		ptp->dev_clk_regs.dev_clk_ns_h,
> +		ptp->dev_clk_regs.phy_clk_ns_l,
> +		ptp->dev_clk_regs.phy_clk_ns_h,
> +		ptp->dev_clk_regs.cmd_sync,
> +	};
> +
> +	ptp->get_dev_clk_time_access =
> +		idpf_ptp_regs_access(ptp, clk_regs, ARRAY_SIZE(clk_regs),
> +				     VIRTCHNL2_CAP_PTP_GET_DEVICE_CLK_TIME_MB);
> +
>  cross_tstamp:
>  	access_type = ptp->get_cross_tstamp_access;
>  	if (access_type != IDPF_PTP_DIRECT)
> @@ -107,10 +147,23 @@ int idpf_ptp_get_caps(struct idpf_adapter *adapter)
>  	ptp->dev_clk_regs.cmd_sync =
>  		libie_pci_get_mmio_addr(mmio, temp_offset);
>  
> +	/* Cross timestamping also samples the device clock registers. */
> +	void __iomem * const cross_regs[] = {
> +		ptp->dev_clk_regs.dev_clk_ns_l,
> +		ptp->dev_clk_regs.dev_clk_ns_h,
> +		ptp->dev_clk_regs.sys_time_ns_l,
> +		ptp->dev_clk_regs.sys_time_ns_h,
> +		ptp->dev_clk_regs.cmd_sync,
> +	};
> +
> +	ptp->get_cross_tstamp_access =
> +		idpf_ptp_regs_access(ptp, cross_regs, ARRAY_SIZE(cross_regs),
> +				     VIRTCHNL2_CAP_PTP_GET_CROSS_TIME_MB);
> +
>  discipline_clock:
>  	access_type = ptp->adj_dev_clk_time_access;
>  	if (access_type != IDPF_PTP_DIRECT)
> -		goto free_resp;
> +		goto check_features_access;
>  
>  	clk_adj_offsets = recv_ptp_caps_msg->clk_adj_offsets;
>  
> @@ -143,13 +196,35 @@ int idpf_ptp_get_caps(struct idpf_adapter *adapter)
>  	ptp->dev_clk_regs.phy_shadj_h =
>  		libie_pci_get_mmio_addr(mmio, temp_offset);
>  
> +	/* Both the main and the PHY timer are disciplined together. */
> +	void __iomem * const adj_regs[] = {
> +		ptp->dev_clk_regs.cmd,
> +		ptp->dev_clk_regs.incval_l,
> +		ptp->dev_clk_regs.incval_h,
> +		ptp->dev_clk_regs.shadj_l,
> +		ptp->dev_clk_regs.shadj_h,
> +		ptp->dev_clk_regs.phy_cmd,
> +		ptp->dev_clk_regs.phy_incval_l,
> +		ptp->dev_clk_regs.phy_incval_h,
> +		ptp->dev_clk_regs.phy_shadj_l,
> +		ptp->dev_clk_regs.phy_shadj_h,
> +	};
> +
> +	ptp->adj_dev_clk_time_access =
> +		idpf_ptp_regs_access(ptp, adj_regs, ARRAY_SIZE(adj_regs),
> +				     VIRTCHNL2_CAP_PTP_ADJ_DEVICE_CLK_MB);
> +
> +	/* Reconcile dependencies after address validation changed access modes. */
> +check_features_access:
> +	idpf_ptp_check_features_access(adapter);

[Severity: High]
Could this leave ptp->dev_clk_regs.cmd_sync as NULL while
ptp->get_dev_clk_time_access, ptp->adj_dev_clk_time_access, and
ptp->set_dev_clk_time_access remain set to IDPF_PTP_DIRECT?

In idpf_ptp_get_caps(), when direct access is negotiated for reading the
device clock, cross-timestamping, and adjusting the clock:

1. ptp->dev_clk_regs.cmd_sync is mapped from clock_offsets.cmd_sync_trigger
   and validated in clk_regs[], leaving ptp->get_dev_clk_time_access as
   IDPF_PTP_DIRECT.

2. Under cross_tstamp, ptp->dev_clk_regs.cmd_sync is unconditionally
   overwritten using cross_tstamp_offsets.cmd_sync_trigger:

	temp_offset = le32_to_cpu(cross_tstamp_offsets.cmd_sync_trigger);
	ptp->dev_clk_regs.cmd_sync =
		libie_pci_get_mmio_addr(mmio, temp_offset);

   If libie_pci_get_mmio_addr() returns NULL for
   cross_tstamp_offsets.cmd_sync_trigger, the previously validated
   ptp->dev_clk_regs.cmd_sync pointer is clobbered with NULL.

3. idpf_ptp_regs_access(ptp, cross_regs, ...) only downgrades
   ptp->get_cross_tstamp_access, while ptp->get_dev_clk_time_access stays
   IDPF_PTP_DIRECT.

4. Under discipline_clock, adj_regs[] does not check
   ptp->dev_clk_regs.cmd_sync, and idpf_ptp_check_features_access() sees
   ptp->get_dev_clk_time_access == IDPF_PTP_DIRECT and
   ptp->adj_dev_clk_time_access == IDPF_PTP_DIRECT.

When idpf_ptp_init() or a PTP clock callback subsequently invokes
idpf_ptp_adj_dev_clk_fine_direct(), idpf_ptp_set_dev_clk_time_direct(),
idpf_ptp_adj_dev_clk_time_direct(), or idpf_ptp_read_src_clk_reg_direct(),
idpf_ptp_tmr_cmd() and idpf_ptp_enable_shtime() will call writel() on a NULL
ptp->dev_clk_regs.cmd_sync pointer.

Also, if clk_regs[] validation downgrades ptp->get_dev_clk_time_access from
IDPF_PTP_DIRECT, should idpf_ptp_get_caps() skip mapping cross_tstamp and
discipline_clock registers instead of mapping them before
check_features_access downgrades those dependent features?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261102110717.77800-1-przemyslaw.korba@intel.com?part=1