Re: [PATCH v2 wireless] wifi: wfx: validate num_tx_confs in rx_helper before use

["Jérôme Pouiller" <jerome.pouiller@silabs.com>]

On Wednesday 4 November 2026 12:04:08 Central European Standard Time Tristan Madani wrote:
> rx_helper() reads num_tx_confs from a firmware multi-transmit
> confirmation and uses it to update tx_buffers_used before the message
> is dispatched to wfx_hif_multi_tx_confirm(), which iterates over the
> tx_conf_payload flexible array.  Neither path checks that the message
> is large enough to hold the claimed number of entries.
> 
> Add a struct_size() check in rx_helper() before num_tx_confs is first
> used, so that both the buffer accounting and the subsequent iteration
> operate on validated data.
> 
> Fixes: 9bca45f3d692 ("staging: wfx: allow to send 802.11 frames")
> Cc: stable@vger.kernel.org
> Signed-off-by: Tristan Madani <tristan@talencesecurity.com>
> ---
> Changes in v2:
> - Moved validation from wfx_hif_multi_tx_confirm() in hif_rx.c to
>   rx_helper() in bh.c, where num_tx_confs is first used for buffer
>   accounting, so the check fires before any side effects (Jérôme Pouiller)
> - Used struct_size() for overflow-safe payload length computation
> 
>  drivers/net/wireless/silabs/wfx/bh.c | 20 ++++++++++++++++----
>  1 file changed, 16 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/wireless/silabs/wfx/bh.c b/drivers/net/wireless/silabs/wfx/bh.c
> index 0071659bc9c98..d4c904324edb7 100644
> --- a/drivers/net/wireless/silabs/wfx/bh.c
> +++ b/drivers/net/wireless/silabs/wfx/bh.c
> @@ -99,11 +99,23 @@ static int rx_helper(struct wfx_dev *wdev, size_t read_len, int *is_cnf)
> 
>         if (!(hif->id & HIF_ID_IS_INDICATION)) {
>                 (*is_cnf)++;
> -               if (hif->id == HIF_CNF_ID_MULTI_TRANSMIT)
> -                       release_count =
> -                               ((struct wfx_hif_cnf_multi_transmit *)hif->body)->num_tx_confs;
> -               else
> +               if (hif->id == HIF_CNF_ID_MULTI_TRANSMIT) {
> +                       const struct wfx_hif_cnf_multi_transmit *body =
> +                               (const struct wfx_hif_cnf_multi_transmit *)hif->body;
> +                       size_t needed = struct_size(body, tx_conf_payload,
> +                                                  body->num_tx_confs);
> +
> +                       if (le16_to_cpu(hif->len) < sizeof(*hif) + needed) {
> +                               dev_err(wdev->dev,
> +                                       "invalid num_tx_confs %d for msg len %d\n",
> +                                       body->num_tx_confs,
> +                                       le16_to_cpu(hif->len));
> +                               goto err;
> +                       }
> +                       release_count = body->num_tx_confs;
> +               } else {

[Cc: Aamir who sent a similar patch]

Until now, I have considered the device is not controlled by third-party
and the device won't try to trick the host. The few existing sanity
checks only notify the developer if the connection is garbage.

In all cases, the errors are not recoverable. A misalignement of the
host and the device state (ie. the release_count variable) will
lead to various lock-up. I believe it should not impact the rest
of the kernel, but a careful review would be required.

So, if I understand right the idea of this patch, if an attacker is
able to get control the device, he will be able to break the WiFi
connectivity, but he won't be able leak data of the kernel. That's
it?

In this case, two things need to be done:
  - Ensure that the various error paths you are creating won't lead to
    a general crash of the kernel. Otherwise, we only increase the
    complexity of our code, decrease the ability to find bug and we are
    still vulnerable.
  - This location is not sufficient to prevent the buffer overflows.
    We need to check the buffer size in many other places (see
    hif_handlers table). It won't make sense to fix this location and
    keep all the other ones open.

[...]

-- 
Jérôme Pouiller