| 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
| ||
|
Message-ID: <d630ca3e-4070-5395-54c2-e20e0c552dc3@lwfinger.net>
Date: Sat, 17 Nov 2018 21:31:35 -0600
From: Larry Finger <Larry.Finger@...inger.net>
To: Priit Laes <plaes@...es.org>, Kees Cook <keescook@...omium.org>,
Jia-Ju Bai <baijiaju1990@...il.com>,
Kalle Valo <kvalo@...eaurora.org>,
"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 11/14/18 12:27 PM, Priit Laes wrote:
> Kernel library has a common cordic algorithm which is identical
> to internally implementatd one, so use it and drop the duplicate
> implementation.
>
> Signed-off-by: Priit Laes <plaes@...es.org>
> ---
> drivers/net/wireless/broadcom/b43/Kconfig | 1 +-
> drivers/net/wireless/broadcom/b43/phy_common.c | 47 +-------------------
> drivers/net/wireless/broadcom/b43/phy_common.h | 9 +----
> drivers/net/wireless/broadcom/b43/phy_lp.c | 13 ++---
> drivers/net/wireless/broadcom/b43/phy_n.c | 13 ++---
> 5 files changed, 15 insertions(+), 68 deletions(-)
>
> diff --git a/drivers/net/wireless/broadcom/b43/Kconfig b/drivers/net/wireless/broadcom/b43/Kconfig
> index fba8560..3e41457 100644
> --- a/drivers/net/wireless/broadcom/b43/Kconfig
> +++ b/drivers/net/wireless/broadcom/b43/Kconfig
> @@ -4,6 +4,7 @@ config B43
> select BCMA if B43_BCMA
> select SSB if B43_SSB
> select FW_LOADER
> + select CORDIC
> ---help---
> b43 is a driver for the Broadcom 43xx series wireless devices.
>
> diff --git a/drivers/net/wireless/broadcom/b43/phy_common.c b/drivers/net/wireless/broadcom/b43/phy_common.c
> index 85f2ca9..98c4fa5 100644
> --- a/drivers/net/wireless/broadcom/b43/phy_common.c
> +++ b/drivers/net/wireless/broadcom/b43/phy_common.c
> @@ -604,50 +604,3 @@ void b43_phy_force_clock(struct b43_wldev *dev, bool force)
> #endif
> }
> }
> -
> -/* http://bcm-v4.sipsolutions.net/802.11/PHY/Cordic */
> -struct b43_c32 b43_cordic(int theta)
> -{
> - static const u32 arctg[] = {
> - 2949120, 1740967, 919879, 466945, 234379, 117304,
> - 58666, 29335, 14668, 7334, 3667, 1833,
> - 917, 458, 229, 115, 57, 29,
> - };
> - u8 i;
> - s32 tmp;
> - s8 signx = 1;
> - u32 angle = 0;
> - struct b43_c32 ret = { .i = 39797, .q = 0, };
> -
> - while (theta > (180 << 16))
> - theta -= (360 << 16);
> - while (theta < -(180 << 16))
> - theta += (360 << 16);
> -
> - if (theta > (90 << 16)) {
> - theta -= (180 << 16);
> - signx = -1;
> - } else if (theta < -(90 << 16)) {
> - theta += (180 << 16);
> - signx = -1;
> - }
> -
> - for (i = 0; i <= 17; i++) {
> - if (theta > angle) {
> - tmp = ret.i - (ret.q >> i);
> - ret.q += ret.i >> i;
> - ret.i = tmp;
> - angle += arctg[i];
> - } else {
> - tmp = ret.i + (ret.q >> i);
> - ret.q -= ret.i >> i;
> - ret.i = tmp;
> - angle -= arctg[i];
> - }
> - }
> -
> - ret.i *= signx;
> - ret.q *= signx;
> -
> - return ret;
> -}
> diff --git a/drivers/net/wireless/broadcom/b43/phy_common.h b/drivers/net/wireless/broadcom/b43/phy_common.h
> index 57a1ad8..4213cac 100644
> --- a/drivers/net/wireless/broadcom/b43/phy_common.h
> +++ b/drivers/net/wireless/broadcom/b43/phy_common.h
> @@ -7,13 +7,6 @@
>
> struct b43_wldev;
>
> -/* Complex number using 2 32-bit signed integers */
> -struct b43_c32 { s32 i, q; };
> -
> -#define CORDIC_CONVERT(value) (((value) >= 0) ? \
> - ((((value) >> 15) + 1) >> 1) : \
> - -((((-(value)) >> 15) + 1) >> 1))
> -
> /* PHY register routing bits */
> #define B43_PHYROUTE 0x0C00 /* PHY register routing bits mask */
> #define B43_PHYROUTE_BASE 0x0000 /* Base registers */
> @@ -450,6 +443,4 @@ bool b43_is_40mhz(struct b43_wldev *dev);
>
> void b43_phy_force_clock(struct b43_wldev *dev, bool force);
>
> -struct b43_c32 b43_cordic(int theta);
> -
> #endif /* LINUX_B43_PHY_COMMON_H_ */
> diff --git a/drivers/net/wireless/broadcom/b43/phy_lp.c b/drivers/net/wireless/broadcom/b43/phy_lp.c
> index 6922cbb..1718e3b 100644
> --- a/drivers/net/wireless/broadcom/b43/phy_lp.c
> +++ b/drivers/net/wireless/broadcom/b43/phy_lp.c
> @@ -23,6 +23,7 @@
>
> */
>
> +#include <linux/cordic.h>
> #include <linux/slab.h>
>
> #include "b43.h"
> @@ -1780,9 +1781,9 @@ static void lpphy_start_tx_tone(struct b43_wldev *dev, s32 freq, u16 max)
> {
> struct b43_phy_lp *lpphy = dev->phy.lp;
> u16 buf[64];
> - int i, samples = 0, angle = 0;
> + int i, samples = 0, theta = 0;
> int rotation = (((36 * freq) / 20) << 16) / 100;
> - struct b43_c32 sample;
> + struct cordic_iq sample;
>
> lpphy->tx_tone_freq = freq;
>
> @@ -1798,10 +1799,10 @@ static void lpphy_start_tx_tone(struct b43_wldev *dev, s32 freq, u16 max)
> }
>
> for (i = 0; i < samples; i++) {
> - sample = b43_cordic(angle);
> - angle += rotation;
> - buf[i] = CORDIC_CONVERT((sample.i * max) & 0xFF) << 8;
> - buf[i] |= CORDIC_CONVERT((sample.q * max) & 0xFF);
> + sample = cordic_calc_iq(theta);
> + theta += rotation;
> + buf[i] = CORDIC_FLOAT((sample.i * max) & 0xFF) << 8;
> + buf[i] |= CORDIC_FLOAT((sample.q * max) & 0xFF);
> }
>
> b43_lptab_write_bulk(dev, B43_LPTAB16(5, 0), samples, buf);
> diff --git a/drivers/net/wireless/broadcom/b43/phy_n.c b/drivers/net/wireless/broadcom/b43/phy_n.c
> index 44ab080..1f9378a 100644
> --- a/drivers/net/wireless/broadcom/b43/phy_n.c
> +++ b/drivers/net/wireless/broadcom/b43/phy_n.c
> @@ -23,6 +23,7 @@
>
> */
>
> +#include <linux/cordic.h>
> #include <linux/delay.h>
> #include <linux/slab.h>
> #include <linux/types.h>
> @@ -1513,7 +1514,7 @@ static void b43_radio_init2055(struct b43_wldev *dev)
>
> /* http://bcm-v4.sipsolutions.net/802.11/PHY/N/LoadSampleTable */
> static int b43_nphy_load_samples(struct b43_wldev *dev,
> - struct b43_c32 *samples, u16 len) {
> + struct cordic_iq *samples, u16 len) {
> struct b43_phy_n *nphy = dev->phy.n;
> u16 i;
> u32 *data;
> @@ -1544,7 +1545,7 @@ static u16 b43_nphy_gen_load_samples(struct b43_wldev *dev, u32 freq, u16 max,
> {
> int i;
> u16 bw, len, rot, angle;
> - struct b43_c32 *samples;
> + struct cordic_iq *samples;
>
> bw = b43_is_40mhz(dev) ? 40 : 20;
> len = bw << 3;
> @@ -1561,7 +1562,7 @@ static u16 b43_nphy_gen_load_samples(struct b43_wldev *dev, u32 freq, u16 max,
> len = bw << 1;
> }
>
> - samples = kcalloc(len, sizeof(struct b43_c32), GFP_KERNEL);
> + samples = kcalloc(len, sizeof(struct cordic_iq), GFP_KERNEL);
> if (!samples) {
> b43err(dev->wl, "allocation for samples generation failed\n");
> return 0;
> @@ -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.
From what I see of the two algorithms, the method is identical once the
differences in scaling are handled. Even so, applying this patch to b43 leads to
a series of B43 error messages followed by a crash in kfree.
Just to add to the level of rejection: NACK.
Larry
Powered by blists - more mailing lists