netdev - Re: [PATCH net] bnxt: fix crashes when reducing ring count with active RSS contexts

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CALs4sv3S_G6k7T2R3GGHy-zNqfr6E2aXMQk1crZytqhJxHOY-A@mail.gmail.com>
Date: Fri, 5 Jul 2024 15:45:42 +0530
From: Pavan Chebbi <pavan.chebbi@...adcom.com>
To: Jakub Kicinski <kuba@...nel.org>
Cc: davem@...emloft.net, netdev@...r.kernel.org, edumazet@...gle.com, 
	pabeni@...hat.com, michael.chan@...adcom.com, 
	kalesh-anakkur.purayil@...adcom.com
Subject: Re: [PATCH net] bnxt: fix crashes when reducing ring count with
 active RSS contexts

On Fri, Jul 5, 2024 at 7:30 AM Jakub Kicinski <kuba@...nel.org> wrote:
>
> bnxt doesn't check if a ring is used by RSS contexts when reducing
> ring count. Core performs a similar check for the drivers for
> the main context, but core doesn't know about additional contexts,
> so it can't validate them. bnxt_fill_hw_rss_tbl_p5() uses ring
> id to index bp->rx_ring[], which without the check may end up
> being out of bounds.
>
>   BUG: KASAN: slab-out-of-bounds in __bnxt_hwrm_vnic_set_rss+0xb79/0xe40
>   Read of size 2 at addr ffff8881c5809618 by task ethtool/31525
>   Call Trace:
>   __bnxt_hwrm_vnic_set_rss+0xb79/0xe40
>    bnxt_hwrm_vnic_rss_cfg_p5+0xf7/0x460
>    __bnxt_setup_vnic_p5+0x12e/0x270
>    __bnxt_open_nic+0x2262/0x2f30
>    bnxt_open_nic+0x5d/0xf0
>    ethnl_set_channels+0x5d4/0xb30
>    ethnl_default_set_doit+0x2f1/0x620
>
> Core does track the additional contexts in net-next, so we can
> move this validation out of the driver as a follow up there.
>
> Fixes: b3d0083caf9a ("bnxt_en: Support RSS contexts in ethtool .{get|set}_rxfh()")
> Signed-off-by: Jakub Kicinski <kuba@...nel.org>
> ---
> CC: michael.chan@...adcom.com
> CC: pavan.chebbi@...adcom.com
> CC: kalesh-anakkur.purayil@...adcom.com
> ---
>  drivers/net/ethernet/broadcom/bnxt/bnxt.c         | 15 +++++++++++++++
>  drivers/net/ethernet/broadcom/bnxt/bnxt.h         |  1 +
>  drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c |  6 ++++++
>  3 files changed, 22 insertions(+)
>
> diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> index 220d05e2f6fa..80fce0aaad66 100644
> --- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> @@ -6282,6 +6282,21 @@ static u16 bnxt_get_max_rss_ring(struct bnxt *bp)
>         return max_ring;
>  }
>
> +u16 bnxt_get_max_rss_ctx_ring(struct bnxt *bp)
> +{
> +       u16 i, tbl_size, max_ring = 0;
> +       struct bnxt_rss_ctx *rss_ctx;
> +
> +       tbl_size = bnxt_get_rxfh_indir_size(bp->dev);
> +
> +       list_for_each_entry(rss_ctx, &bp->rss_ctx_list, list) {
> +               for (i = 0; i < tbl_size; i++)
> +                       max_ring = max(max_ring, rss_ctx->rss_indir_tbl[i]);
> +       }
> +
> +       return max_ring;
> +}
> +
>  int bnxt_get_nr_rss_ctxs(struct bnxt *bp, int rx_rings)
>  {
>         if (bp->flags & BNXT_FLAG_CHIP_P5_PLUS) {
> diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.h b/drivers/net/ethernet/broadcom/bnxt/bnxt.h
> index e46bd11e52b0..3c8826875ceb 100644
> --- a/drivers/net/ethernet/broadcom/bnxt/bnxt.h
> +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.h
> @@ -2814,6 +2814,7 @@ int bnxt_hwrm_vnic_set_tpa(struct bnxt *bp, struct bnxt_vnic_info *vnic,
>  void bnxt_fill_ipv6_mask(__be32 mask[4]);
>  int bnxt_alloc_rss_indir_tbl(struct bnxt *bp, struct bnxt_rss_ctx *rss_ctx);
>  void bnxt_set_dflt_rss_indir_tbl(struct bnxt *bp, struct bnxt_rss_ctx *rss_ctx);
> +u16 bnxt_get_max_rss_ctx_ring(struct bnxt *bp);
>  int bnxt_get_nr_rss_ctxs(struct bnxt *bp, int rx_rings);
>  int bnxt_hwrm_vnic_cfg(struct bnxt *bp, struct bnxt_vnic_info *vnic);
>  int bnxt_hwrm_vnic_alloc(struct bnxt *bp, struct bnxt_vnic_info *vnic,
> diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c
> index bf157f6cc042..4d53ec7adc61 100644
> --- a/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c
> +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c
> @@ -961,6 +961,12 @@ static int bnxt_set_channels(struct net_device *dev,
>                 return rc;
>         }
>
> +       if (req_rx_rings < bp->rx_nr_rings &&
> +           req_rx_rings <= bnxt_get_max_rss_ctx_ring(bp)) {
> +               netdev_warn(dev, "Can't deactivate rings used by RSS contexts\n");
> +               return -EINVAL;
> +       }
> +
>         if (bnxt_get_nr_rss_ctxs(bp, req_rx_rings) !=
>             bnxt_get_nr_rss_ctxs(bp, bp->rx_nr_rings) &&
>             netif_is_rxfh_configured(dev)) {
> --
> 2.45.2
>
Thanks Jakub for the patch. This is much better than my earlier
thought of prioritizing ring change by destroying all the RSS ctxs.
Patch LGTM.
Reviewed-by: Pavan Chebbi <pavan.chebbi@...adcom.com>

Download attachment "smime.p7s" of type "application/pkcs7-signature" (4209 bytes)