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
| ||
|
Date: Thu, 23 May 2019 10:51:01 +0200 From: Arnd Bergmann <arnd@...db.de> To: Nathan Chancellor <natechancellor@...il.com> Cc: Nick Desaulniers <ndesaulniers@...gle.com>, Amitkumar Karwar <amitkarwar@...il.com>, Siva Rebbagondla <siva8118@...il.com>, Kalle Valo <kvalo@...eaurora.org>, linux-wireless <linux-wireless@...r.kernel.org>, Networking <netdev@...r.kernel.org>, LKML <linux-kernel@...r.kernel.org>, clang-built-linux <clang-built-linux@...glegroups.com> Subject: Re: [PATCH] rsi: Properly initialize data in rsi_sdio_ta_reset On Thu, May 23, 2019 at 3:54 AM Nathan Chancellor <natechancellor@...il.com> wrote: > > On Thu, May 02, 2019 at 08:17:18PM -0700, Nathan Chancellor wrote: > > On Thu, May 02, 2019 at 11:18:01AM -0700, Nick Desaulniers wrote: > > > On Thu, May 2, 2019 at 8:16 AM Nathan Chancellor > > > u8 data [4]; > > > > data was __le32 in rsi_reset_chip() before commit f700546682a6 ("rsi: > > fix nommu_map_sg overflow kernel panic"). > > > > I wonder if this would be okay for this function: > > > > ------------------------------------------------- > > > > diff --git a/drivers/net/wireless/rsi/rsi_91x_sdio.c b/drivers/net/wireless/rsi/rsi_91x_sdio.c > > index f9c67ed473d1..0330c50ab99c 100644 > > --- a/drivers/net/wireless/rsi/rsi_91x_sdio.c > > +++ b/drivers/net/wireless/rsi/rsi_91x_sdio.c > > @@ -927,7 +927,7 @@ static int rsi_sdio_ta_reset(struct rsi_hw *adapter) > > { > > int status; > > u32 addr; > > - u8 *data; > > + u8 data; > > > > status = rsi_sdio_master_access_msword(adapter, TA_BASE_ADDR); > > if (status < 0) { > > @@ -937,7 +937,7 @@ static int rsi_sdio_ta_reset(struct rsi_hw *adapter) > > } > > > > rsi_dbg(INIT_ZONE, "%s: Bring TA out of reset\n", __func__); > > - put_unaligned_le32(TA_HOLD_THREAD_VALUE, data); > > + put_unaligned_le32(TA_HOLD_THREAD_VALUE, &data); > > addr = TA_HOLD_THREAD_REG | RSI_SD_REQUEST_MASTER; > > status = rsi_sdio_write_register_multiple(adapter, addr, > > (u8 *)&data, This is clearly not ok, as put_unaligned_le32() stores four bytes, and the local variable is only one byte! Also, sdio does use DMA for transfers, so the variable has to be dynamically allocated. I think your original patch was correct. The only change I'd possibly make would be to use RSI_9116_REG_SIZE instead of sizeof(u32). > Did any of the maintainers have any comments on what the correct > solution is here to resolve this warning? It is one of the few left > before we can turn on -Wuninitialized for the whole kernel. I would argue that this should not stop us from turning it on, as the warning is for a clear bug in the code that absolutely needs to be fixed, rather than a false-positive. Arnd
Powered by blists - more mailing lists