netdev - Re: [PATCH net 2/9] bnx2x: Prevent an illegal pointer dereference during panic

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: <1382035647.20308.3.camel@lb-tlvb-eilong.il.broadcom.com>
Date:	Thu, 17 Oct 2013 21:47:27 +0300
From:	"Eilon Greenstein" <eilong@...adcom.com>
To:	"David Miller" <davem@...emloft.net>
cc:	yuvalmin@...adcom.com, netdev@...r.kernel.org, ariele@...adcom.com,
	dmitry@...adcom.com
Subject: Re: [PATCH net 2/9] bnx2x: Prevent an illegal pointer
 dereference during panic

On Thu, 2013-10-17 at 14:21 -0400, David Miller wrote:
> From: "Yuval Mintz" <yuvalmin@...adcom.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@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html