netdev - Re: [PATCH net-next] cxgb4: remove bogus CHELSIO_VPD_UNIQUE

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: <85fe1327-3f75-c480-c5e2-0045877188ce@gmail.com>
Date:   Wed, 20 Jan 2021 08:07:32 +0100
From:   Heiner Kallweit <hkallweit1@...il.com>
To:     Jakub Kicinski <kuba@...nel.org>
Cc:     Raju Rangoju <rajur@...lsio.com>,
        David Miller <davem@...emloft.net>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>
Subject: Re: [PATCH net-next] cxgb4: remove bogus CHELSIO_VPD_UNIQUE_ID
 constant

On 19.01.2021 23:02, Jakub Kicinski wrote:
> On Sat, 16 Jan 2021 14:45:25 +0100 Heiner Kallweit wrote:
>> The comment is quite weird, there is no such thing as a vendor-specific
>> VPD id. 0x82 is the value of PCI_VPD_LRDT_ID_STRING. So what we are
>> doing here is simply checking whether the byte at VPD address VPD_BASE
>> is a valid string LRDT, same as what is done a few lines later in
>> the code.
>> LRDT = Large Resource Data Tag, see PCI 2.2 spec, VPD chapter
>>
>> Signed-off-by: Heiner Kallweit <hkallweit1@...il.com>
> 
> Did you find this by code inspection?
> 
Well, more or less ..
RTL8168 indicates it has VPD but in all cases I've seen there is no
VPD EEPROM. Result is that the VPD code throws an "invalid VPD tag" error.
When checking the VPD code (+ VPD spec) and its (few) users I came across
the chelsio driver.

>> diff --git a/drivers/net/ethernet/chelsio/cxgb4/t4_hw.c b/drivers/net/ethernet/chelsio/cxgb4/t4_hw.c
>> index 2c80371f9..48f20a6a0 100644
>> --- a/drivers/net/ethernet/chelsio/cxgb4/t4_hw.c
>> +++ b/drivers/net/ethernet/chelsio/cxgb4/t4_hw.c
>> @@ -2689,7 +2689,6 @@ void t4_get_regs(struct adapter *adap, void *buf, size_t buf_size)
>>  #define VPD_BASE           0x400
>>  #define VPD_BASE_OLD       0
>>  #define VPD_LEN            1024
>> -#define CHELSIO_VPD_UNIQUE_ID 0x82
>>  
>>  /**
>>   * t4_eeprom_ptov - translate a physical EEPROM address to virtual
>> @@ -2743,9 +2742,9 @@ int t4_seeprom_wp(struct adapter *adapter, bool enable)
>>   */
>>  int t4_get_raw_vpd_params(struct adapter *adapter, struct vpd_params *p)
>>  {
>> -	int i, ret = 0, addr;
>> +	int i, ret = 0, addr = VPD_BASE;
> 
> IMHO it's more readable if the addr is set to BASE or BASE_OLD in one
> place rather than having a default value at variable init which may be
> overwritten.
> 
OK. Thought was just that VPD_BASE is the more common case.
I'll change this in a v2.

>>  	int ec, sn, pn, na;
>> -	u8 *vpd, csum;
>> +	u8 *vpd, csum, base_val = 0;
>>  	unsigned int vpdr_len, kw_offset, id_len;
>>  
>>  	vpd = vmalloc(VPD_LEN);
>> @@ -2755,17 +2754,12 @@ int t4_get_raw_vpd_params(struct adapter *adapter, struct vpd_params *p)
>>  	/* Card information normally starts at VPD_BASE but early cards had
>>  	 * it at 0.
>>  	 */
>> -	ret = pci_read_vpd(adapter->pdev, VPD_BASE, sizeof(u32), vpd);
>> +	ret = pci_read_vpd(adapter->pdev, VPD_BASE, 1, &base_val);
> 
> Are we sure this works? I've seen silicon out there which has problems
> with small PCI accesses (granted those were not VPD accesses).
> 
The underlying PCI register access reads 4 bytes, the VPD code will ignore
the higher three bytes here.

>>  	if (ret < 0)
>>  		goto out;
>>  
>> -	/* The VPD shall have a unique identifier specified by the PCI SIG.
>> -	 * For chelsio adapters, the identifier is 0x82. The first byte of a VPD
>> -	 * shall be CHELSIO_VPD_UNIQUE_ID (0x82). The VPD programming software
>> -	 * is expected to automatically put this entry at the
>> -	 * beginning of the VPD.
>> -	 */
>> -	addr = *vpd == CHELSIO_VPD_UNIQUE_ID ? VPD_BASE : VPD_BASE_OLD;
>> +	if (base_val != PCI_VPD_LRDT_ID_STRING)
>> +		addr = VPD_BASE_OLD;
>>  
>>  	ret = pci_read_vpd(adapter->pdev, addr, VPD_LEN, vpd);
>>  	if (ret < 0)
>