netdev - Re: [PATCH v2] qed: fix qed_fill

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: <20160601.220614.2104618620006713727.davem@davemloft.net>
Date:	Wed, 01 Jun 2016 22:06:14 -0700 (PDT)
From:	David Miller <davem@...emloft.net>
To:	Yuval.Mintz@...gic.com
Cc:	arnd@...db.de, manish.chopra@...gic.com,
	Sudarsana.Kalluru@...gic.com, netdev@...r.kernel.org,
	linux-kernel@...r.kernel.org, Ariel.Elior@...gic.com
Subject: Re: [PATCH v2] qed: fix qed_fill_link() error handling

From: Yuval Mintz <Yuval.Mintz@...gic.com>
Date: Wed, 1 Jun 2016 13:36:38 +0000

>> gcc warns about qed_fill_link possibly accessing uninitialized data:
>> 
>> drivers/net/ethernet/qlogic/qed/qed_main.c: In function 'qed_fill_link':
>> drivers/net/ethernet/qlogic/qed/qed_main.c:1170:35: error: 'link_caps' may be
>> used uninitialized in this function [-Werror=maybe-uninitialized]
>> 
>> While this warning is only about the specific case of CONFIG_QED_SRIOV being
>> disabled but the function getting called for a VF (which should never happen),
>> another possibility is that qed_mcp_get_*() fails without returning data.
>> 
>> This rearranges the code so we bail out in either of the two cases and print a
>> warning instead of accessing the uninitialized data.
>> 
>> The qed_link_output structure remains untouched in this case, but all callers first
>> call memset() on it, so at least we are not leaking stack data then.
>> 
>> As discussed, we also use a compile-time check to ensure we never use any of
>> the VF code if CONFIG_QED_SRIOV is disabled, and the PCI device table is
>> updated to no longer bind to virtual functions in that configuration.
>> 
>> Signed-off-by: Arnd Bergmann <arnd@...db.de>
>> 
>> ---
>> On Wednesday, June 1, 2016 11:10:30 AM CEST Yuval Mintz wrote:
>> > Actually, I think VF probe should gracefully fail in that case, as
>> > qed_vf_hw_prepare() would simply return -EINVAL.
>> > But I can honestly say I've never tested this flow, and I agree
>> > there's no reason to allow VF probe in case we're not supporting SRIOV.
>> 
>> ok
>> 
>> > So I guess removing the PCI ID and defining IS_PF to be true in case
>> > CONFIG_QED_SRIOV isn't set is the right way to go.
>> > Do you want to revise your patch, or do you want me to do it?
>> 
>> I've done the patch below now, please either Ack or modify it the way you like
>> and forward it.
>> 
>> Thanks,
>> 
>> 	Arnd
> 
> Perhaps it would have made more sense as a 2-part series; But I'm content with
> the changes themselves. I'd let Dave decide whether he wants it split.
> 
> Thanks for taking the time fixing this.
> 
> Acked-by: Yuval Mintz <Yuval.Mintz@...gic.com>

I think this is fine as one change, because the two changes are
inetricably linked together in my opinion.  Either you nop out these
VF code paths and SRIOV is disabled and don't match the VF IDs, or you
don't.  There really isn't a valid intermediate state.

So, applied, thanks everyone.