[v2,1/3] net/rnp: add check firmware respond info

Add logic checks at critical points to detect potentially illegal
firmware information, preventing subsequent logic exceptions.

Fixes: 52aae4ed4ffb ("net/rnp: add device capabilities")
Fixes: 52dfb84e14be ("net/rnp: add device init and uninit")
Cc: stable@dpdk.org

Signed-off-by: Wenbo Cao <caowenbo@mucse.com>
Reviewed-by: Stephen Hemminger <stephen@networkplumber.org>
---
 drivers/net/rnp/base/rnp_mbx_fw.c | 15 ++++++++++++++-
 drivers/net/rnp/rnp_ethdev.c      | 16 ++++++++--------
 2 files changed, 22 insertions(+), 9 deletions(-)
  

On Mon, 30 Jun 2025 13:57:51 +0800
Wenbo Cao <caowenbo@mucse.com> wrote:

> Add logic checks at critical points to detect potentially illegal
> firmware information, preventing subsequent logic exceptions.
> 
> Fixes: 52aae4ed4ffb ("net/rnp: add device capabilities")
> Fixes: 52dfb84e14be ("net/rnp: add device init and uninit")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Wenbo Cao <caowenbo@mucse.com>
> Reviewed-by: Stephen Hemminger <stephen@networkplumber.org>

>  int rnp_mbx_fw_get_capability(struct rnp_eth_port *port)
>  {
>  	struct rnp_phy_abilities_rep ability;
> @@ -252,17 +253,29 @@ int rnp_mbx_fw_get_capability(struct rnp_eth_port *port)
>  		hw->nic_mode = ability.nic_mode;
>  		/* get phy<->lane mapping info */
>  		lane_cnt = rte_popcount32(hw->lane_mask);
> +		if (lane_cnt > RNP_MAX_PORT_OF_PF) {
> +			RNP_PMD_LOG(ERR, "firmware invalid lane_mask");
> +			return -EINVAL;
> +		}
>  		temp_mask = hw->lane_mask;
> +		if (temp_mask == 0 || temp_mask > RNP_MAX_LANE_MASK) {
> +			RNP_PMD_LOG(ERR, "lane_mask is invalid 0x%.2x", temp_mask);
> +			return -EINVAL;
> +		}
>  		if (ability.e.ports_is_sgmii_valid)
>  			is_sgmii_bits = ability.e.lane_is_sgmii;
>  		for (idx = 0; idx < lane_cnt; idx++) {
>  			hw->phy_port_ids[idx] = port_ids[idx];
> +			if (temp_mask == 0) {
> +				RNP_PMD_LOG(ERR, "temp_mask is zero at idx=%d", idx);
> +				return -EINVAL;
> +			}
>  			lane_bit = ffs(temp_mask) - 1;
>  			lane_idx = port_ids[idx] % lane_cnt;
>  			hw->lane_of_port[lane_idx] = lane_bit;
>  			is_sgmii = lane_bit & is_sgmii_bits ? 1 : 0;
>  			hw->lane_is_sgmii[lane_idx] = is_sgmii;
> -			temp_mask &= ~RTE_BIT32(lane_bit);
> +			temp_mask &= ~(1ULL << lane_bit);

Rather than using ffs directly better to consistently use rte_bitops
which has rte_ffs32().  You can address it in a future version.
  

Hi Stephen,

Thank you for your review. Regarding the changes to the rte_bitops API, 
should these updates be implemented in V3 or deferred to the next release
version?

Regards Wenbo
> -----Original Message-----
> From: Stephen Hemminger <stephen@networkplumber.org>
> Sent: 2025年6月30日 23:07
> To: Wenbo Cao <caowenbo@mucse.com>
> Cc: Ferruh Yigit <ferruh.yigit@amd.com>; dev@dpdk.org; yaojun@mucse.com;
> stable@dpdk.org
> Subject: Re: [PATCH v2 1/3] net/rnp: add check firmware respond info
> 
> On Mon, 30 Jun 2025 13:57:51 +0800
> Wenbo Cao <caowenbo@mucse.com> wrote:
> 
> > Add logic checks at critical points to detect potentially illegal
> > firmware information, preventing subsequent logic exceptions.
> >
> > Fixes: 52aae4ed4ffb ("net/rnp: add device capabilities")
> > Fixes: 52dfb84e14be ("net/rnp: add device init and uninit")
> > Cc: stable@dpdk.org
> >
> > Signed-off-by: Wenbo Cao <caowenbo@mucse.com>
> > Reviewed-by: Stephen Hemminger <stephen@networkplumber.org>
> 
> >  int rnp_mbx_fw_get_capability(struct rnp_eth_port *port)  {
> >  	struct rnp_phy_abilities_rep ability; @@ -252,17 +253,29 @@ int
> > rnp_mbx_fw_get_capability(struct rnp_eth_port *port)
> >  		hw->nic_mode = ability.nic_mode;
> >  		/* get phy<->lane mapping info */
> >  		lane_cnt = rte_popcount32(hw->lane_mask);
> > +		if (lane_cnt > RNP_MAX_PORT_OF_PF) {
> > +			RNP_PMD_LOG(ERR, "firmware invalid lane_mask");
> > +			return -EINVAL;
> > +		}
> >  		temp_mask = hw->lane_mask;
> > +		if (temp_mask == 0 || temp_mask > RNP_MAX_LANE_MASK) {
> > +			RNP_PMD_LOG(ERR, "lane_mask is invalid 0x%.2x",
> temp_mask);
> > +			return -EINVAL;
> > +		}
> >  		if (ability.e.ports_is_sgmii_valid)
> >  			is_sgmii_bits = ability.e.lane_is_sgmii;
> >  		for (idx = 0; idx < lane_cnt; idx++) {
> >  			hw->phy_port_ids[idx] = port_ids[idx];
> > +			if (temp_mask == 0) {
> > +				RNP_PMD_LOG(ERR, "temp_mask is zero at
> idx=%d", idx);
> > +				return -EINVAL;
> > +			}
> >  			lane_bit = ffs(temp_mask) - 1;
> >  			lane_idx = port_ids[idx] % lane_cnt;
> >  			hw->lane_of_port[lane_idx] = lane_bit;
> >  			is_sgmii = lane_bit & is_sgmii_bits ? 1 : 0;
> >  			hw->lane_is_sgmii[lane_idx] = is_sgmii;
> > -			temp_mask &= ~RTE_BIT32(lane_bit);
> > +			temp_mask &= ~(1ULL << lane_bit);
> 
> Rather than using ffs directly better to consistently use rte_bitops which
has
> rte_ffs32().  You can address it in a future version.
>