netdev - Re: [PATCH net-next] net: phy: lan87xx: fix access to wrong register of LAN87xx

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: <53b244ad343f4fa9533ff5fa1e2b0b25ba92a984.camel@microchip.com>
Date:   Mon, 29 Mar 2021 12:07:28 +0000
From:   <Andre.Edich@...rochip.com>
To:     <mans@...sr.com>
CC:     <Parthiban.Veerasooran@...rochip.com>, <netdev@...r.kernel.org>,
        <UNGLinuxDriver@...rochip.com>
Subject: Re: [PATCH net-next] net: phy: lan87xx: fix access to wrong register
 of LAN87xx

On Mon, 2021-03-29 at 12:19 +0100, Måns Rullgård wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know
> the content is safe
> 
> Andre Edich <andre.edich@...rochip.com> writes:
> 
> > The function lan87xx_config_aneg_ext was introduced to configure
> > LAN95xxA but as well writes to undocumented register of LAN87xx.
> > This fix prevents that access.
> > 
> > The function lan87xx_config_aneg_ext gets more suitable for the new
> > behavior name.
> > 
> > Reported-by: Måns Rullgård <mans@...sr.com>
> > Fixes: 05b35e7eb9a1 ("smsc95xx: add phylib support")
> > Signed-off-by: Andre Edich <andre.edich@...rochip.com>
> > ---
> >  drivers/net/phy/smsc.c | 7 +++++--
> >  1 file changed, 5 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/net/phy/smsc.c b/drivers/net/phy/smsc.c
> > index ddb78fb4d6dc..d8cac02a79b9 100644
> > --- a/drivers/net/phy/smsc.c
> > +++ b/drivers/net/phy/smsc.c
> > @@ -185,10 +185,13 @@ static int lan87xx_config_aneg(struct
> > phy_device *phydev)
> >       return genphy_config_aneg(phydev);
> >  }
> > 
> > -static int lan87xx_config_aneg_ext(struct phy_device *phydev)
> > +static int lan95xx_config_aneg_ext(struct phy_device *phydev)
> >  {
> >       int rc;
> > 
> > +     if (phydev->phy_id != 0x0007c0f0) /* not (LAN9500A or LAN9505A)
> > */
> > +             return lan87xx_config_aneg(phydev);
> > +
> >       /* Extend Manual AutoMDIX timer */
> >       rc = phy_read(phydev, PHY_EDPD_CONFIG);
> >       if (rc < 0)
> > @@ -441,7 +444,7 @@ static struct phy_driver smsc_phy_driver[] = {
> >       .read_status    = lan87xx_read_status,
> >       .config_init    = smsc_phy_config_init,
> >       .soft_reset     = smsc_phy_reset,
> > -     .config_aneg    = lan87xx_config_aneg_ext,
> > +     .config_aneg    = lan95xx_config_aneg_ext,
> > 
> >       /* IRQ related */
> >       .config_intr    = smsc_phy_config_intr,
> > --
> 
> This seems to differentiate based on the "revision" field of the ID
> register.  Can we be certain that a future update of chip won't break
> this assumption?

The way to fail would be to "fix" and release any of LAN95xxA in the
way that the register map will is changed or feature is disabled but
the Phy ID  remains the same.  This is very unlikely and obviously
wrong.  I don't believe this may happen.

If a new chip with the different Phy ID but the same feature will be
released, then it must be explicitly added into the table.

Is there a third case I don't see?

-- 
Regards,
Andre

> 
> --
> Måns Rullgård