linux-kernel - Re: [PATCH v3 3/3] b43: Use cordic algorithm from kernel library

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: <20181119111422.6vbxnmo4lgm6kl55@plaes.org>
Date:   Mon, 19 Nov 2018 11:14:22 +0000
From:   Priit Laes <plaes@...es.org>
To:     Kalle Valo <kvalo@...eaurora.org>
Cc:     Larry Finger <Larry.Finger@...inger.net>,
        Kees Cook <keescook@...omium.org>,
        Jia-Ju Bai <baijiaju1990@...il.com>,
        "Gustavo A. R. Silva" <gustavo@...eddedor.com>,
        Colin Ian King <colin.king@...onical.com>,
        Arend van Spriel <arend.vanspriel@...adcom.com>,
        Varsha Rao <rvarsha016@...il.com>,
        linux-wireless@...r.kernel.org, b43-dev@...ts.infradead.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 3/3] b43: Use cordic algorithm from kernel library

On Mon, Nov 19, 2018 at 12:43:32PM +0200, Kalle Valo wrote:
> Larry Finger <Larry.Finger@...inger.net> writes:
> 
> >>>> @@ -1570,10 +1571,10 @@ static u16 b43_nphy_gen_load_samples(struct b43_wldev *dev, u32 freq, u16 max,
> >>>>    	angle = 0;
> >>>>    	for (i = 0; i < len; i++) {
> >>>> -		samples[i] = b43_cordic(angle);
> >>>> +		samples[i] = cordic_calc_iq(angle);
> >>>>    		angle += rot;
> >>>> -		samples[i].q = CORDIC_CONVERT(samples[i].q * max);
> >>>> -		samples[i].i = CORDIC_CONVERT(samples[i].i * max);
> >>>> +		samples[i].q = CORDIC_FLOAT(samples[i].q * max);
> >>>> +		samples[i].i = CORDIC_FLOAT(samples[i].i * max);
> >>>>    	}
> >>>>    	i = b43_nphy_load_samples(dev, samples, len);
> >>>
> >>> There is a fundamental flaw in this patch. Routine b43_cordic() takes an
> >>> angle in degrees scaled by 2^16, whereas cordic_calc_iq() takes an angle in
> >>> degrees. For a given input, the two routines must get different answers. At
> >>> a minimum, the calculation of rot would need to remove the left shift of 16.
> >>
> >> Thanks for the hint. I modified my "test harness" a bit to plot out values
> >> from -360 .. 360 and transformed the theta for b43_cordic argument
> >> using CORDIC_FIXED macro:
> >>
> >> b43_cordic(CORDIC_FIXED(theta));
> >> cordic_calc_iq(theta);
> >>
> >> Then I plotted the results and well.. they are not that pretty.
> >> While the results give
> >> identical answers between certain ranges of degrees, the cordic
> >> algorithm for b43 seems
> >> to be broken for certain ranges: (-270..-180 ; -90 .. 0; 90.. 180 and 270..360).
> >>
> >> You can find my test harnesses here:
> >>
> >> https://gist.github.com/plaes/284993a4fc65e0926d0628a11f0cf874
> >
> > I found a problem with the b43 implementation. The local variables for
> > that routine includes
> >
> >         u32 angle = 0;
> >
> > If one looks further down in the algorithm, if the reduced value of
> > "theta" is less than 0, then "angle" should be negative. That causes
> > the calculation to blow up. This explains why some ranges of angles
> > got the same result for both routines. When that declaration is
> > changed to "int angle = 0", the two routines give the same answer for
> > all inputs.
> >
> > My test setup has a hardware failure, thus I cannot test your patch,
> > but I now believe it to be correct. Thus your first and third patches
> > may be annotated with
> > ACKed-by: Larry Finger <Larry.Finger@...inger.net>
> >
> > One thing that should be done is to fix the error in the b43 code for
> > stable as it was introduced in 2.6.34. I propose adding the attached
> > patched to your series placed between your current 2nd and 3rd patches
> > so that the old kernels get fixed. Of course, your 3rd patch will need
> > to be revised. If all 4 of the patches get submitted together there
> > will be no problems with the timing. My change will exist for seconds
> > in the mainline kernel, but it will get propagated back through
> > stable.
> 
> Sorry Larry, I'm not fully understanding what you mean here. So I'm
> going to just drop the whole series and assume that Priit will submit a
> new version. Please let me know if I should do something else.

Yes, drop this one and I will submit v4 with one extra patch fixing the
cordic algorithm in the stable kernel.

> 
> -- 
> Kalle Valo