[net,2/9] bnx2x: Prevent an illegal pointer dereference during panic

From: Dmitry Kravkov <dmitry@broadcom.com>

During a panic, the driver tries to print the Management FW buffer of recent
commands. To do so, the driver reads the address of that buffer from a known
address. If the buffer is unavailable (e.g., PCI reads don't work, MCP is
failing, etc.), the driver will try to access the address it has read, possibly
causing a kernel panic.

This check 'sanitizes' the access, validating the read value is indeed a valid
address inside the management FW's buffers.

Signed-off-by: Dmitry Kravkov <dmitry@broadcom.com>
Signed-off-by: Yuval Mintz <yuvalmin@broadcom.com>
Signed-off-by: Ariel Elior <ariele@broadcom.com>
Signed-off-by: Eilon Greenstein <eilong@broadcom.com>
---
 drivers/net/ethernet/broadcom/bnx2x/bnx2x.h      |  4 ++++
 drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c | 19 ++++++++++++++-----
 2 files changed, 18 insertions(+), 5 deletions(-)

From: "Yuval Mintz" <yuvalmin@broadcom.com>
Date: Tue, 15 Oct 2013 16:28:48 +0200

> @@ -775,6 +775,15 @@ void bnx2x_fw_dump_lvl(struct bnx2x *bp, const char *lvl)
>  		trace_shmem_base = bp->common.shmem_base;
>  	else
>  		trace_shmem_base = SHMEM2_RD(bp, other_shmem_base_addr);
> +
> +	/* sanity */
> +	if (trace_shmem_base < MCPR_SCRATCH_BASE(bp) ||
> +	    trace_shmem_base > MCPR_SCRATCH_BASE(bp) + 0x28000) {

I would say that this second test should be ">=" rather than ">".

Actually, there are a lot of holes still remaining here.

trace_shmem_base is validated, but then you access the signature at
0x800 bytes before trace_shmem_base value.  That should be accounted
for in the test above too.

And what about that 'mark' value you read?  Any validations needed on
that?

And then you read from "addr" to "mark", and I see no checks that this
range makes any sense either.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Thu, 2013-10-17 at 14:21 -0400, David Miller wrote:
> From: "Yuval Mintz" <yuvalmin@broadcom.com>
> Date: Tue, 15 Oct 2013 16:28:48 +0200
> 
> > @@ -775,6 +775,15 @@ void bnx2x_fw_dump_lvl(struct bnx2x *bp, const char *lvl)
> >  		trace_shmem_base = bp->common.shmem_base;
> >  	else
> >  		trace_shmem_base = SHMEM2_RD(bp, other_shmem_base_addr);
> > +
> > +	/* sanity */
> > +	if (trace_shmem_base < MCPR_SCRATCH_BASE(bp) ||
> > +	    trace_shmem_base > MCPR_SCRATCH_BASE(bp) + 0x28000) {
> 
> I would say that this second test should be ">=" rather than ">".
> 
> Actually, there are a lot of holes still remaining here.
> 
> trace_shmem_base is validated, but then you access the signature at
> 0x800 bytes before trace_shmem_base value.  That should be accounted
> for in the test above too.
> 
> And what about that 'mark' value you read?  Any validations needed on
> that?
> 
> And then you read from "addr" to "mark", and I see no checks that this
> range makes any sense either.

The failure that we saw was due to PCI read failure, so we could have
checked that we did not read all 1's, but then we thought about a
stronger validation to make sure this address is not being overrun for
some unknown reason (maybe stack overflow of the FW, after all, we are
at a FW bug handling routine) so this added condition validate that the
address is within the scratchpad limitation. To make it more precious,
we should account for the differences between the different silicon
revisions, but we did not really see such a failure - the failure we saw
was reading all 1's, the extra sanity will catch some more failure, but
obviously not all.

I guess we should really re-spin and check for all 1's, or add some more
code to make it really accurate, or at least fix the ">" to ">=". We
will send a revised series with a fix.

Thanks,
Eilon


--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

From: "Eilon Greenstein" <eilong@broadcom.com>
Date: Thu, 17 Oct 2013 21:47:27 +0300

> I guess we should really re-spin and check for all 1's, or add some more
> code to make it really accurate, or at least fix the ">" to ">=". We
> will send a revised series with a fix.

Thanks Eilon.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html