netdev - Re: [PATCH net v5] net: systemport: Add error pointer checks in bcm_sysport_map_queues() and bcm_sysport_unmap

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: <CAEKBCKP2pGoy=CWpzn+NGq8r4biu=XVnszXQ=7Ruuan8rfxM1Q@mail.gmail.com>
Date: Mon, 30 Sep 2024 23:52:45 +0545
From: Dipendra Khadka <kdipendra88@...il.com>
To: Vladimir Oltean <vladimir.oltean@....com>
Cc: Simon Horman <horms@...nel.org>, florian.fainelli@...adcom.com, 
	bcm-kernel-feedback-list@...adcom.com, davem@...emloft.net, 
	edumazet@...gle.com, kuba@...nel.org, pabeni@...hat.com, 
	maxime.chevallier@...tlin.com, netdev@...r.kernel.org, 
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH net v5] net: systemport: Add error pointer checks in
 bcm_sysport_map_queues() and bcm_sysport_unmap_queues()

Hi Vladimir,

On Fri, 27 Sept 2024 at 17:45, Vladimir Oltean <vladimir.oltean@....com> wrote:
>
> On Fri, Sep 27, 2024 at 02:29:58PM +0300, Vladimir Oltean wrote:
> > > > + dp = dsa_port_from_netdev(slave_dev);
> > > > + if (IS_ERR(dp))
> > > > +         return PTR_ERR(dp);
> >
> > I don't see an explanation anywhere as for why dsa_port_from_netdev()
> > could ever return a pointer-encoded error here? hmm? Did you follow the
> > call path and found a problem?
>

Yeah, you are right. I ran smatch to find this and saw there is no
validation. I did not see any problem as you said. I thought it would
be better to include this change. If you say there is no point for
this change, then that's also fine for me. I got the chance to learn
new things.

> To make my point even clearer. As the code goes:
>
> bool dsa_user_dev_check(const struct net_device *dev)
> {
>         // This dereferences "dev" without a NULL pointer check.
>         // If the kernel did not crash, it means that "dev" is not null.
>         return dev->netdev_ops == &dsa_user_netdev_ops;
> }
>
> static int bcm_sysport_netdevice_event(struct notifier_block *nb,
>                                        unsigned long event, void *ptr)
> {
>         ...
>         switch (event) {
>         case NETDEV_CHANGEUPPER:
>                 ...
>                 if (!dsa_user_dev_check(info->upper_dev))
>                         return NOTIFY_DONE;
>
>                 // we know here that dsa_user_dev_check() is true, and
>                 // no one changes dev->netdev_ops at runtime, to suspect
>                 // it could become false after it just returned true.
>                 // Even if it did, we are under rtnl_lock(), and whoever
>                 // did that better also acquired rtnl_lock(). Thus,
>                 // there is enough guarantee that this also remains true
>                 // below.
>                 if (info->linking)
>                         ret = bcm_sysport_map_queues(dev, info->upper_dev);
>                 else
>                         ret = bcm_sysport_unmap_queues(dev, info->upper_dev);
>         }
>         ...
> }
>
> struct dsa_port *dsa_port_from_netdev(struct net_device *netdev)
> {
>         if (!netdev || !dsa_user_dev_check(netdev))
>                 return ERR_PTR(-ENODEV);
>
>         return dsa_user_to_port(netdev);
> }
>
> static int bcm_sysport_map_queues(struct net_device *dev,
>                                   struct net_device *slave_dev)
> {
>         struct dsa_port *dp = dsa_port_from_netdev(slave_dev);
>         ...
> }
>
> So, if both conditions for dsa_port_from_netdev() to return ERR_PTR(-ENODEV)
> can only be false, why would we add an error check? Is it to appease a
> static analysis tool which doesn't analyze things very far? Or is there
> an actual problem?
>
> And why does this have a Fixes: tag and the expectation to be included
> as a bug fix to stable kernels?
>
> And why is the author of the blamed patch even CCed only at v5?!

Sorry to know this, I ran the script and there I did not find your name.

./scripts/get_maintainer.pl drivers/net/ethernet/broadcom/bcmsysport.c
Florian Fainelli <florian.fainelli@...adcom.com> (supporter:BROADCOM
SYSTEMPORT ETHERNET DRIVER)
Broadcom internal kernel review list
<bcm-kernel-feedback-list@...adcom.com> (reviewer:BROADCOM SYSTEMPORT
ETHERNET DRIVER)
"David S. Miller" <davem@...emloft.net> (maintainer:NETWORKING DRIVERS)
Eric Dumazet <edumazet@...gle.com> (maintainer:NETWORKING DRIVERS)
Jakub Kicinski <kuba@...nel.org> (maintainer:NETWORKING DRIVERS)
Paolo Abeni <pabeni@...hat.com> (maintainer:NETWORKING DRIVERS)
netdev@...r.kernel.org (open list:BROADCOM SYSTEMPORT ETHERNET DRIVER)
linux-kernel@...r.kernel.org (open list)

Thank you so much for your time and effort , special thanks to Simon
for everything ,thanks Vladimir for the way you explained. and thanks
Florian for your help.

Best regards,
Dipendra Khadka