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

[Thor Lancelot Simon <tls%panix.com@localhost>]

On Sun, Oct 09, 2022 at 09:24:32AM -0500, Mario Campos wrote:
> On Sat, Oct 8, 2022 at 1:00 PM Taylor R Campbell
> <campbell+netbsd-tech-kern%mumble.net@localhost> wrote:
> >
> > > Date: Sat, 8 Oct 2022 10:58:58 -0500
> > > From: Mario Campos <mario.andres.campos%gmail.com@localhost>
> > >
> > > 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.

So, to me this looks like a bit of a lesson on the importance of
understanding what the program being analyzed does, when interpreting
the results of automated analysis tools.

This is a driver for a hardware device.  The variables in question are
used to describe the number of queues available from the hardware.  These
queues are typically allocated per CPU core on the hosting system, at
most, and are definitely bounded by the properties of the underlying
device hardware.  It is not plausible that this hardware device would ever
exceed 16 bits worth of distinct queues (as Taylor notes, 8 is a typical
value).  The use of a 16 bit variable to hold the value likely reflects
the underlying hardware stuffing two values into a single 32-bit register.

Which is not to say this shouldn't be fixed - it should.  But, if the
tool flagged other results they are almost certainly more likely to
reflect actual bugs which could impact a system in the real world!

Thor