netdev - Re: [PATCH 1/2] net: dsa: bcm_sf2: add function finding RGMII register

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]

Date:   Thu, 18 Mar 2021 08:30:26 +0100
From:   Rafał Miłecki <zajec5@...il.com>
To:     Florian Fainelli <f.fainelli@...il.com>,
        Andrew Lunn <andrew@...n.ch>,
        Vivien Didelot <vivien.didelot@...il.com>,
        Vladimir Oltean <olteanv@...il.com>
Cc:     "David S . Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>, netdev@...r.kernel.org,
        bcm-kernel-feedback-list@...adcom.com,
        Rafał Miłecki <rafal@...ecki.pl>
Subject: Re: [PATCH 1/2] net: dsa: bcm_sf2: add function finding RGMII
 register

On 17.03.2021 22:20, Florian Fainelli wrote:
> On 3/17/2021 7:37 AM, Rafał Miłecki wrote:
>> From: Rafał Miłecki <rafal@...ecki.pl>
>>
>> Simple macro like REG_RGMII_CNTRL_P() is insufficient as:
>> 1. It doesn't validate port argument
>> 2. It doesn't support chipsets with non-lineral RGMII regs layout
>>
>> Missing port validation could result in getting register offset from out
>> of array. Random memory -> random offset -> random reads/writes. It
>> affected e.g. BCM4908 for REG_RGMII_CNTRL_P(7).
> 
> That is entirely fair, however as a bug fix this is not necessarily the
> simplest way to approach this.

I'm not sure if I understand. Should I fix it in some totally different
way? Or should I just follow your inline suggestions?


>> Fixes: a78e86ed586d ("net: dsa: bcm_sf2: Prepare for different register layouts")
>> Signed-off-by: Rafał Miłecki <rafal@...ecki.pl>
>> ---
>>   drivers/net/dsa/bcm_sf2.c      | 51 ++++++++++++++++++++++++++++++----
>>   drivers/net/dsa/bcm_sf2_regs.h |  2 --
>>   2 files changed, 45 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/net/dsa/bcm_sf2.c b/drivers/net/dsa/bcm_sf2.c
>> index ba5d546d06aa..942773bcb7e0 100644
>> --- a/drivers/net/dsa/bcm_sf2.c
>> +++ b/drivers/net/dsa/bcm_sf2.c
>> @@ -32,6 +32,30 @@
>>   #include "b53/b53_priv.h"
>>   #include "b53/b53_regs.h"
>>   
>> +static u16 bcm_sf2_reg_rgmii_cntrl(struct bcm_sf2_priv *priv, int port)
> 
> This is not meant to be used outside the file, so I would be keen on
> removing the bcm_sf2_ prefix to make the name shorter and closer to the
> original macro name.

Most or all local functions use such a prefix. E.g.:
bcm_sf2_num_active_ports()
bcm_sf2_recalc_clock()
bcm_sf2_imp_setup()
bcm_sf2_gphy_enable_set()
bcm_sf2_port_intr_enable()
bcm_sf2_port_intr_disable()
bcm_sf2_port_setup()
bcm_sf2_port_disable()

It would be inconsistent to have RGMII reg function not follow that.


>> +{
>> +	switch (priv->type) {
>> +	case BCM4908_DEVICE_ID:
>> +		/* TODO */
>> +		break;
>> +	default:
>> +		switch (port) {
>> +		case 0:
>> +			return REG_RGMII_0_CNTRL;
>> +		case 1:
>> +			return REG_RGMII_1_CNTRL;
>> +		case 2:
>> +			return REG_RGMII_2_CNTRL;
>> +		default:
>> +			break;
>> +		}
>> +	}
>> +
>> +	WARN_ONCE(1, "Unsupported port %d\n", port);
>> +
>> +	return 0;
> 
> maybe return -1 or -EINVAL just in case 0 happens to be a valid offset
> in the future. Checking the return value is not necessarily going to be
> helpful as it needs immediate fixing, so what we could do is keep the
> WARN_ON, and return the offset of REG_SWITCH_STATUS which is a read-only
> register. This will trigger the bus arbiter logic to return an error
> because a write was attempted from a read-only register.
> 
> What do you think?

Great, thanks!