Re: Comparison of different-width ints in ixg(4)

[Mario Campos]

On Sat, Oct 8, 2022 at 1:00 PM Taylor R Campbell
 wrote:
>
> > Date: Sat, 8 Oct 2022 10:58:58 -0500
> > From: Mario Campos 
> >
> > I ran a SAST tool, CodeQL, against trunk and found a couple of
> > instances (below) where the 16-bit integer `i` is compared to the
> > 32-bit integer `max_rx_queues` or `max_tx_queues` in ixg(4). If
> > `max_rx_queues` (or `max_tx_queues`) is sufficiently large, it could
> > lead to an infinite loop.
> >
> > sys/dev/pci/ixgbe/ixgbe_vf.c:280
> > sys/dev/pci/ixgbe/ixgbe_vf.c:284
> > sys/dev/pci/ixgbe/ixgbe_common.c:1158
> > sys/dev/pci/ixgbe/ixgbe_common.c:1162
>
> Cool.  I don't think this case is a bug because the quantities in
> question are bounded by IXGBE_VF_MAX_TX/RX_QUEUES, which are both 8.
> But it would be reasonable to use u32 or even just unsigned for this.
> Did this tool turn anything else up?

Ah, great! I also think it would still be an improvement, if only as a
means of defensive programming should IXGBE_VF_MAX_TX/RX_QUEUES ever
be increased.

There were actually over 3400+ results -- though I haven't sifted
through all of it yet. If anyone is interested in reading the report,
I can share it as a SARIF (basically, JSON) file. Or I can grant
access to my GitHub repo, mario-campos/netbsd, where I performed the
scan. Of course, a third option is to replicate my work (in the repo)
and reproduce the results.

Comparison of different-width ints in ixg(4)

[Mario Campos]

Hello.

I ran a SAST tool, CodeQL, against trunk and found a couple of
instances (below) where the 16-bit integer `i` is compared to the
32-bit integer `max_rx_queues` or `max_tx_queues` in ixg(4). If
`max_rx_queues` (or `max_tx_queues`) is sufficiently large, it could
lead to an infinite loop.

sys/dev/pci/ixgbe/ixgbe_vf.c:280
sys/dev/pci/ixgbe/ixgbe_vf.c:284
sys/dev/pci/ixgbe/ixgbe_common.c:1158
sys/dev/pci/ixgbe/ixgbe_common.c:1162

**NOTE: this patch needs to be tested, as I have not tested it beyond
re-compiling the kernel.**

diff --git a/sys/dev/pci/ixgbe/ixgbe_common.c b/sys/dev/pci/ixgbe/ixgbe_common.c
index 04aac162f..b3b14e263 100644
--- a/sys/dev/pci/ixgbe/ixgbe_common.c
+++ b/sys/dev/pci/ixgbe/ixgbe_common.c
@@ -1115,7 +1115,7 @@ void ixgbe_set_lan_id_multi_port_pcie(struct ixgbe_hw *hw)
 s32 ixgbe_stop_adapter_generic(struct ixgbe_hw *hw)
 {
u32 reg_val;
-   u16 i;
+   u32 i;

DEBUGFUNC("ixgbe_stop_adapter_generic");

diff --git a/sys/dev/pci/ixgbe/ixgbe_vf.c b/sys/dev/pci/ixgbe/ixgbe_vf.c
index 63fceacdd..924ac2963 100644
--- a/sys/dev/pci/ixgbe/ixgbe_vf.c
+++ b/sys/dev/pci/ixgbe/ixgbe_vf.c
@@ -262,7 +262,7 @@ s32 ixgbe_reset_hw_vf(struct ixgbe_hw *hw)
 s32 ixgbe_stop_adapter_vf(struct ixgbe_hw *hw)
 {
u32 reg_val;
-   u16 i;
+   u32 i;

/*
 * Set the adapter_stopped flag so other driver functions stop touching