netdev - RE: [patch net-next v4 4/4] igb/igbvf: implement ndo_get_phys_port

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: <C5551D9AAB213A418B7FD5E4A6F30A077466D9D6@ORSMSX108.amr.corp.intel.com>
Date:	Thu, 25 Jul 2013 19:23:00 +0000
From:	"Rose, Gregory V" <gregory.v.rose@...el.com>
To:	"Duyck, Alexander H" <alexander.h.duyck@...el.com>,
	Ben Hutchings <bhutchings@...arflare.com>
CC:	Jiri Pirko <jiri@...nulli.us>,
	"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
	"davem@...emloft.net" <davem@...emloft.net>,
	"stephen@...workplumber.org" <stephen@...workplumber.org>,
	"Narendra_K@...l.com" <Narendra_K@...l.com>,
	"Fastabend, John R" <john.r.fastabend@...el.com>,
	"or.gerlitz@...il.com" <or.gerlitz@...il.com>,
	"Kirsher, Jeffrey T" <jeffrey.t.kirsher@...el.com>,
	"Brandeburg, Jesse" <jesse.brandeburg@...el.com>,
	"Allan, Bruce W" <bruce.w.allan@...el.com>,
	"Wyborny, Carolyn" <carolyn.wyborny@...el.com>,
	"Skidmore, Donald C" <donald.c.skidmore@...el.com>,
	"Waskiewicz Jr, Peter P" <peter.p.waskiewicz.jr@...el.com>,
	"Ronciak, John" <john.ronciak@...el.com>,
	"Dave, Tushar N" <tushar.n.dave@...el.com>,
	"Vick, Matthew" <matthew.vick@...el.com>,
	"Williams, Mitch A" <mitch.a.williams@...el.com>,
	"vyasevic@...hat.com" <vyasevic@...hat.com>,
	"amwang@...hat.com" <amwang@...hat.com>,
	"johannes@...solutions.net" <johannes@...solutions.net>
Subject: RE: [patch net-next v4 4/4] igb/igbvf: implement
 ndo_get_phys_port_id

> -----Original Message-----
> From: Duyck, Alexander H
> Sent: Thursday, July 25, 2013 12:04 PM
> To: Ben Hutchings
> Cc: Jiri Pirko; netdev@...r.kernel.org; davem@...emloft.net;
> stephen@...workplumber.org; Narendra_K@...l.com; Fastabend, John R;
> or.gerlitz@...il.com; Kirsher, Jeffrey T; Brandeburg, Jesse; Allan, Bruce
> W; Wyborny, Carolyn; Skidmore, Donald C; Rose, Gregory V; Waskiewicz Jr,
> Peter P; Ronciak, John; Dave, Tushar N; Vick, Matthew; Williams, Mitch A;
> vyasevic@...hat.com; amwang@...hat.com; johannes@...solutions.net
> Subject: Re: [patch net-next v4 4/4] igb/igbvf: implement
> ndo_get_phys_port_id
> 
> On 07/25/2013 11:51 AM, Ben Hutchings wrote:
> > On Thu, 2013-07-25 at 11:00 -0700, Alexander Duyck wrote:
> >> On 07/25/2013 09:44 AM, Ben Hutchings wrote:
> >>> On Thu, 2013-07-25 at 09:17 -0700, Alexander Duyck wrote:
> >>>> On 07/25/2013 06:03 AM, Jiri Pirko wrote:
> >>>>> @@ -1982,6 +2001,21 @@ static s32 igb_init_i2c(struct igb_adapter
> *adapter)
> >>>>>  	return status;
> >>>>>  }
> >>>>>
> >>>>> +static void igb_compute_phys_port_id(struct igb_adapter *adapter)
> >>>>> +{
> >>>>> +	adapter->phys_port_id = *((u32 *) adapter->netdev->dev_addr);
> >>>>> +	adapter->phys_port_id ^= *((u32 *) adapter->netdev->dev_addr +
> 4);
> >>>>> +	adapter->phys_port_id ^= (long) adapter;
> >>>>> +	adapter->phys_port_id ^= (long) adapter->hw.hw_addr;
> >>>>> +	adapter->phys_port_id ^= (long) adapter->hw.flash_address;
> >>>>> +	adapter->phys_port_id ^= (u32) adapter->hw.io_base;
> >>>>> +	adapter->phys_port_id ^= adapter->hw.device_id;
> >>>>> +	adapter->phys_port_id ^= adapter->hw.subsystem_vendor_id;
> >>>>> +	adapter->phys_port_id ^= adapter->hw.subsystem_device_id;
> >>>>> +	adapter->phys_port_id ^= adapter->hw.vendor_id;
> >>>>> +	adapter->phys_port_id ^= adapter->hw.revision_id;
> >>> I didn't look at Jiri's patch initially, but... what's wrong with
> >>> using the MAC address from NVRAM (should already be in netdev-
> >perm_addr)?
> >>> That's what I'm expecting to do for the SFC9000 family in sfc.
> >>>
> >>>>> +}
> >>>>> +
> >>>>>  /**
> >>>>>   *  igb_probe - Device Initialization Routine
> >>>>>   *  @pdev: PCI device information struct
> >>>> I really think this bit here should be standardized and made
> >>>> available to all drivers.
> >>> [...]
> >>>
> >>> I think it's a bad example and should not be used in any drivers!
> >>>
> >>> Ben.
> >>>
> >> I agree.  That is why the second paragraph started listing what was
> >> wrong with this implementation.  What I said was meant in the more
> >> general sense that whatever solution should be used should be the
> >> same across multiple drivers, not up to each driver to compute.
> > I would love to know how you think this can be done generically.  If
> > it can then we don't need the driver operation at all.
> >
> > Ben.
> >
> 
> Well like you mentioned, you could just pull this out out of
> netdev->perm_addr.  You don't need to have anything driver specific as
> long as that is the field you are using generic netdev or pci_dev
> attributes to do the identification.  All you need is something that
> uniquely identifies the device in the system correct?  For that matter it
> seems like you could probably just pull the domain, bus, device, and
> function number out of the PCI device and that would probably work as well
> as long as you cannot somehow have PFs running inside of guests.

NACK any direct information to the VF guest device as to what its domain, bus, device and function numbers are.  This opens up various exploits.  Virtual functions are not guaranteed to always run in trusted guest VMs, they shouldn't be given anymore HW information than needed to function.

There's a reason we asked Jiri to use a hash value.

- Greg

> 
> Thanks,
> 
> Alex