netdev - Re: [PATCH] wlcore: consolidate kmalloc + memset 0 into kzalloc

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] [day] [month] [year] [list]

Message-ID: <CAGRGNgUsmx-nHEwykpFP5YbXVwrpNE1tRFs0hTuUea5Xvy1k-Q@mail.gmail.com>
Date:	Tue, 22 Dec 2015 18:52:33 +1100
From:	Julian Calaby <julian.calaby@...il.com>
To:	Nicholas Mc Guire <der.herr@...r.at>
Cc:	Nicholas Mc Guire <hofrat@...dl.org>,
	Kalle Valo <kvalo@...eaurora.org>,
	Eliad Peller <eliad@...ery.com>,
	Johannes Berg <johannes.berg@...el.com>,
	Arik Nemtsov <arik@...ery.com>,
	Tony Lindgren <tony@...mide.com>,
	Luciano Coelho <luca@...lho.fi>,
	Eyal Reizer <eyalreizer@...il.com>,
	Emmanuel Grumbach <emmanuel.grumbach@...el.com>,
	linux-wireless <linux-wireless@...r.kernel.org>,
	netdev <netdev@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] wlcore: consolidate kmalloc + memset 0 into kzalloc

Hi Nicholas,

On Tue, Dec 22, 2015 at 6:29 PM, Nicholas Mc Guire <der.herr@...r.at> wrote:
> On Tue, Dec 22, 2015 at 09:56:10AM +1100, Julian Calaby wrote:
>> Hi,
>>
>> On Tue, Dec 22, 2015 at 3:47 AM, Nicholas Mc Guire <hofrat@...dl.org> wrote:
>> > This is an API consolidation only. The use of kmalloc + memset to 0
>> > is equivalent to kzalloc.
>> >
>> > Signed-off-by: Nicholas Mc Guire <hofrat@...dl.org>
>> > ---
>> >
>> > Found by coccinelle script (relaxed version of
>> > scripts/coccinelle/api/alloc/kzalloc-simple.cocci)
>> >
>> > Patch was compile tested with: x86_64_defconfig +
>> > CONFIG_WL12XX=m (implies CONFIG_WLCORE=m)
>> >
>> > Patch is against linux-next (localversion-next is -next-20151221)
>> >
>> >  drivers/net/wireless/ti/wlcore/main.c | 3 +--
>> >  1 file changed, 1 insertion(+), 2 deletions(-)
>> >
>> > diff --git a/drivers/net/wireless/ti/wlcore/main.c b/drivers/net/wireless/ti/wlcore/main.c
>> > index ec7f6af..dfc49bf 100644
>> > --- a/drivers/net/wireless/ti/wlcore/main.c
>> > +++ b/drivers/net/wireless/ti/wlcore/main.c
>> > @@ -838,7 +838,7 @@ static void wl12xx_read_fwlog_panic(struct wl1271 *wl)
>> >
>> >         wl1271_info("Reading FW panic log");
>> >
>> > -       block = kmalloc(wl->fw_mem_block_size, GFP_KERNEL);
>> > +       block = kzalloc(wl->fw_mem_block_size, GFP_KERNEL);
>> >         if (!block)
>> >                 return;
>> >
>> > @@ -885,7 +885,6 @@ static void wl12xx_read_fwlog_panic(struct wl1271 *wl)
>> >                         goto out;
>> >                 }
>> > -               memset(block, 0, wl->fw_mem_block_size);
>>
>> I don't think you can't remove this line. It appears that the loop
>> this is part of resets block to be all zero, reads a chunk of data in,
>> then operates on it. I'm guessing that the code after the following
>> line expects that there isn't any data left over from previous runs
>> through the loop.
>>
> the rational for this being ok is thta the copy operation into block is:
>                 ret = wlcore_read_hwaddr(wl, addr, block,
>                                         wl->fw_mem_block_size, false);
>
> this will end up in the .read methods where block should be completely
> overwritten (length == full block size), so within the loop if successful
> this should be correct - if not successful it would "goto out" witout
> using the content of block.
>
> Am I overlooking something here ?

It's quite possible I am. I didn't look at the implementation of
wlcore_read_hwaddr() so I'm only guessing that it could do partial
reads.

That said, if it does overwrite the entire buffer each time, then
there's no need to use kzalloc() to allocate the buffer in the first
place.

Either way, it needs a review from someone more familiar with the code.

Thanks,

-- 
Julian Calaby

Email: julian.calaby@...il.com
Profile: http://www.google.com/profiles/julian.calaby/
--
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