linux-kernel - Re: [PATCH] staging: brcm80211: return false if not a broadcom board

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: <20110616233736.GA2344@broadcom.com>
Date:	Thu, 16 Jun 2011 16:37:36 -0700
From:	"Henry Ptasinski" <henryp@...adcom.com>
To:	"Greg Dietsche" <gregory.dietsche@....edu>
cc:	"gregkh@...e.de" <gregkh@...e.de>,
	"Brett Rudley" <brudley@...adcom.com>,
	"Dowan Kim" <dowan@...adcom.com>,
	"Roland Vossen" <rvossen@...adcom.com>,
	"Arend Van Spriel" <arend@...adcom.com>,
	"linux-wireless@...r.kernel.org" <linux-wireless@...r.kernel.org>,
	"devel@...verdev.osuosl.org" <devel@...verdev.osuosl.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"Henry Ptasinski" <henryp@...adcom.com>
Subject: Re: [PATCH] staging: brcm80211: return false if not a broadcom
 board

On Wed, Jun 15, 2011 at 07:49:06PM -0700, Greg Dietsche wrote:
> 
> 
> On 06/15/2011 08:36 PM, Henry Ptasinski wrote:
> > On Wed, Jun 15, 2011 at 03:25:09PM -0700, Greg Dietsche wrote:
> >    
> >> This code looks wrong to me. I think it meant to return false
> >> if the board's vendor id isn't Broadcom's.
> >>
> >> Compile tested only.
> >>
> >> Signed-off-by: Greg Dietsche<Gregory.Dietsche@....edu>
> >> ---
> >>   drivers/staging/brcm80211/brcmsmac/wlc_bmac.c |    2 +-
> >>   1 files changed, 1 insertions(+), 1 deletions(-)
> >>
> >> diff --git a/drivers/staging/brcm80211/brcmsmac/wlc_bmac.c b/drivers/staging/brcm80211/brcmsmac/wlc_bmac.c
> >> index 4534926..ee13238 100644
> >> --- a/drivers/staging/brcm80211/brcmsmac/wlc_bmac.c
> >> +++ b/drivers/staging/brcm80211/brcmsmac/wlc_bmac.c
> >> @@ -1943,7 +1943,7 @@ static bool wlc_validboardtype(struct wlc_hw_info *wlc_hw)
> >>   	}
> >>
> >>   	if (wlc_hw->sih->boardvendor != PCI_VENDOR_ID_BROADCOM)
> >> -		return goodboard;
> >> +		goodboard = false;
> >>
> >>   	return goodboard;
> >>   }
> >>      
> > Actually, the original code is correct.
> >
> > For boards with the Broadcom ID, this function checks that the boardrev is
> > sane. Dev boards that aren't properly configured may have invalid info, so this
> > check is mainly to catch that problem.
> >
> > For boards with any other vendor, we don't have any sanity checks that we know
> > should be done, so the board info is always considered good.
> >
> > The device ID is of course always checked.
> >
> > - Henry
> >
> >    
> Hi Henry,
> Thanks for the explanation - I'm still a little confused on this... 
> probably more because of how the code reads. Let me explain:
> 
> The part that threw me off in this code when I originally read it is 
> that it always returns goodboard. It doesn't actually matter what the 
> vendor id is. So my first thought was "what did they really mean to code 
> here...?" That's why I sent the patch. So if the patch isn't necessary, 
> then great! :) But it seems like we could/should just drop the check for 
> vendor id?

How's this for a somewhat clearer implementation:

static bool brcms_c_validboardtype(struct brcms_c_hw_info *wlc_hw)
{
        bool goodboard = true;
        uint boardrev = wlc_hw->boardrev;

        if (wlc_hw->sih->boardvendor == PCI_VENDOR_ID_BROADCOM) {
                /* validate boardrev */
                if (boardrev == 0)
                        goodboard = false;
                else if (boardrev > 0xff) {
                        /* 4 bits each for board type, major, minor, and tiny
                        version numbers */
                        uint brt = (boardrev & 0xf000) >> 12;
                        uint b0 = (boardrev & 0xf00) >> 8;
                        uint b1 = (boardrev & 0xf0) >> 4;
                        uint b2 = boardrev & 0xf;

                        if ((brt > 2) || (brt == 0) || (b0 > 9) || (b0 == 0)
                                || (b1 > 9) || (b2 > 9))
                                goodboard = false;
                }
        }

        return goodboard;
}


If that's agreeable, I'll add it to a series of cleanup patches that I've got
in progress.

- Henry

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/