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 <[email protected]> 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/[email protected]?part=1
