| Message ID | 20240607070613.12156-1-amishin@t-argos.ru (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net] bnxt_en: Adjust logging of firmware messages in case of released token in __hwrm_send() | expand |
On 07.06.2024 09:06, Aleksandr Mishin wrote: > In case of token is released due to token->state == BNXT_HWRM_DEFERRED, > released token (set to NULL) is used in log messages. This issue is > expected to be prevented by HWRM_ERR_CODE_PF_UNAVAILABLE error code. But > this error code is returned by recent firmware. So some firmware may not > return it. This may lead to NULL pointer dereference. > Adjust this issue by adding token pointer check. > > Found by Linux Verification Center (linuxtesting.org) with SVACE. > > Fixes: 8fa4219dba8e ("bnxt_en: add dynamic debug support for HWRM messages") > Signed-off-by: Aleksandr Mishin <amishin@t-argos.ru> > --- Thanks! Reviewed-by: Wojciech Drewek <wojciech.drewek@intel.com> > drivers/net/ethernet/broadcom/bnxt/bnxt_hwrm.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_hwrm.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_hwrm.c > index 1df3d56cc4b5..14585ac476c8 100644 > --- a/drivers/net/ethernet/broadcom/bnxt/bnxt_hwrm.c > +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_hwrm.c > @@ -678,7 +678,7 @@ static int __hwrm_send(struct bnxt *bp, struct bnxt_hwrm_ctx *ctx) > if (rc == HWRM_ERR_CODE_BUSY && !(ctx->flags & BNXT_HWRM_CTX_SILENT)) > netdev_warn(bp->dev, "FW returned busy, hwrm req_type 0x%x\n", > req_type); > - else if (rc && rc != HWRM_ERR_CODE_PF_UNAVAILABLE) > + else if (rc && token && rc != HWRM_ERR_CODE_PF_UNAVAILABLE) > hwrm_err(bp, ctx, "hwrm req_type 0x%x seq id 0x%x error 0x%x\n", > req_type, token->seq_id, rc); > rc = __hwrm_to_stderr(rc);
On Fri, Jun 7, 2024 at 12:08 AM Aleksandr Mishin <amishin@t-argos.ru> wrote: > > In case of token is released due to token->state == BNXT_HWRM_DEFERRED, > released token (set to NULL) is used in log messages. This issue is > expected to be prevented by HWRM_ERR_CODE_PF_UNAVAILABLE error code. But > this error code is returned by recent firmware. So some firmware may not > return it. This may lead to NULL pointer dereference. > Adjust this issue by adding token pointer check. > > Found by Linux Verification Center (linuxtesting.org) with SVACE. > > Fixes: 8fa4219dba8e ("bnxt_en: add dynamic debug support for HWRM messages") > Signed-off-by: Aleksandr Mishin <amishin@t-argos.ru> > --- > drivers/net/ethernet/broadcom/bnxt/bnxt_hwrm.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_hwrm.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_hwrm.c > index 1df3d56cc4b5..14585ac476c8 100644 > --- a/drivers/net/ethernet/broadcom/bnxt/bnxt_hwrm.c > +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_hwrm.c > @@ -678,7 +678,7 @@ static int __hwrm_send(struct bnxt *bp, struct bnxt_hwrm_ctx *ctx) > if (rc == HWRM_ERR_CODE_BUSY && !(ctx->flags & BNXT_HWRM_CTX_SILENT)) > netdev_warn(bp->dev, "FW returned busy, hwrm req_type 0x%x\n", > req_type); > - else if (rc && rc != HWRM_ERR_CODE_PF_UNAVAILABLE) > + else if (rc && token && rc != HWRM_ERR_CODE_PF_UNAVAILABLE) > hwrm_err(bp, ctx, "hwrm req_type 0x%x seq id 0x%x error 0x%x\n", > req_type, token->seq_id, rc); Thanks for the patch. I think doing this will be better to preserve the error message: hwrm_err(bp, ctx, "hwrm req_type 0x%x seq id 0x%x error 0x%x\n", req_type, le16_to_cpu(ctx->req->seq_id), rc); The seq id is saved in ctx->req so we can use it.
diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_hwrm.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_hwrm.c index 1df3d56cc4b5..14585ac476c8 100644 --- a/drivers/net/ethernet/broadcom/bnxt/bnxt_hwrm.c +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_hwrm.c @@ -678,7 +678,7 @@ static int __hwrm_send(struct bnxt *bp, struct bnxt_hwrm_ctx *ctx) if (rc == HWRM_ERR_CODE_BUSY && !(ctx->flags & BNXT_HWRM_CTX_SILENT)) netdev_warn(bp->dev, "FW returned busy, hwrm req_type 0x%x\n", req_type); - else if (rc && rc != HWRM_ERR_CODE_PF_UNAVAILABLE) + else if (rc && token && rc != HWRM_ERR_CODE_PF_UNAVAILABLE) hwrm_err(bp, ctx, "hwrm req_type 0x%x seq id 0x%x error 0x%x\n", req_type, token->seq_id, rc); rc = __hwrm_to_stderr(rc);
In case of token is released due to token->state == BNXT_HWRM_DEFERRED, released token (set to NULL) is used in log messages. This issue is expected to be prevented by HWRM_ERR_CODE_PF_UNAVAILABLE error code. But this error code is returned by recent firmware. So some firmware may not return it. This may lead to NULL pointer dereference. Adjust this issue by adding token pointer check. Found by Linux Verification Center (linuxtesting.org) with SVACE. Fixes: 8fa4219dba8e ("bnxt_en: add dynamic debug support for HWRM messages") Signed-off-by: Aleksandr Mishin <amishin@t-argos.ru> --- drivers/net/ethernet/broadcom/bnxt/bnxt_hwrm.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)