netdev - Re: [PATCH net-next 4/6 v6] net: dsa: rtl8366rb: Fix off-by-one bug

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: <20210925184424.4gfdxmcg2bay575o@skbuf>
Date:   Sat, 25 Sep 2021 21:44:24 +0300
From:   Vladimir Oltean <olteanv@...il.com>
To:     Linus Walleij <linus.walleij@...aro.org>
Cc:     Andrew Lunn <andrew@...n.ch>,
        Vivien Didelot <vivien.didelot@...il.com>,
        Florian Fainelli <f.fainelli@...il.com>,
        "David S . Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>, netdev@...r.kernel.org,
        Mauri Sandberg <sandberg@...lfence.com>,
        Alvin Šipraga <alsi@...g-olufsen.dk>,
        DENG Qingfang <dqfext@...il.com>
Subject: Re: [PATCH net-next 4/6 v6] net: dsa: rtl8366rb: Fix off-by-one bug

On Sat, Sep 25, 2021 at 03:23:09PM +0200, Linus Walleij wrote:
> The max VLAN number with non-4K VLAN activated is 15, and the
> range is 0..15. Not 16.
> 
> The impact should be low since we by default have 4K VLAN and
> thus have 4095 VLANs to play with in this switch. There will
> not be a problem unless the code is rewritten to only use
> 16 VLANs.
> 
> Fixes: d8652956cf37 ("net: dsa: realtek-smi: Add Realtek SMI driver")
> Cc: Vladimir Oltean <olteanv@...il.com>
> Cc: Mauri Sandberg <sandberg@...lfence.com>
> Cc: Alvin Šipraga <alsi@...g-olufsen.dk>
> Cc: DENG Qingfang <dqfext@...il.com>
> Cc: Florian Fainelli <f.fainelli@...il.com>
> Signed-off-by: Linus Walleij <linus.walleij@...aro.org>
> ---
> ChangeLog v5->v6:
> - No changes just resending with the rest of the
>   patches.
> ChangeLog v4->v5:
> - Add some more text describing that this is not a critical bug.
> - Add Fixes tag
> ChangeLog v1->v4:
> - New patch for a bug discovered when fixing the other issues.
> ---
>  drivers/net/dsa/rtl8366rb.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/dsa/rtl8366rb.c b/drivers/net/dsa/rtl8366rb.c
> index 2c66a0c2ee50..6f25ee57069d 100644
> --- a/drivers/net/dsa/rtl8366rb.c
> +++ b/drivers/net/dsa/rtl8366rb.c
> @@ -1450,7 +1450,7 @@ static int rtl8366rb_set_mc_index(struct realtek_smi *smi, int port, int index)
>  
>  static bool rtl8366rb_is_vlan_valid(struct realtek_smi *smi, unsigned int vlan)
>  {
> -	unsigned int max = RTL8366RB_NUM_VLANS;
> +	unsigned int max = RTL8366RB_NUM_VLANS - 1;
>  
>  	if (smi->vlan4k_enabled)
>  		max = RTL8366RB_NUM_VIDS - 1;

Personally I would have preferred to make this "max" = RTL8366RB_NUM_VIDS,
and then the comparison right below:

	if (vlan >= max)
		return false;

because it's easier than to carry around "- 1" everywhere.
Anyway, this works just as well.

Reviewed-by: Vladimir Oltean <olteanv@...il.com>