netdev - RE: [PATCH net-next] net: phy: smsc: add WoL support to LAN8740/LAN8742 PHYs.

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: Wed, 31 May 2023 22:43:01 +0000
From: <Tristram.Ha@...rochip.com>
To: <andrew@...n.ch>
CC: <davem@...emloft.net>, <f.fainelli@...il.com>, <netdev@...r.kernel.org>,
	<UNGLinuxDriver@...rochip.com>
Subject: RE: [PATCH net-next] net: phy: smsc: add WoL support to
 LAN8740/LAN8742 PHYs.

> Subject: Re: [PATCH net-next] net: phy: smsc: add WoL support to LAN8740/LAN8742
> PHYs.
> 
> > +     if (wol->wolopts & WAKE_ARP) {
> > +             const u8 *ip_addr =
> > +                     ((const u8 *)&((ndev->ip_ptr)->ifa_list)->ifa_address);
> 
> I'm not sure this is safe. What happens when the interface only has an
> IPv6 address? Is ifa_list a NULL pointer? I really think you need to
> be using a core helper to get the IPv4 address.
> 

This will be fixed with in_dev_get() and rcu_dereference().
Indeed the address will disappear when the link is down.
Why is that so?

> > +             const u16 mask[3] = { 0xF03F, 0x003F, 0x03C0 };
> 
> Are there any endianness issues here? I've not looked at how mask is
> used, but if it is indicating which bytes in the pattern should be
> matched on, i guess endian does matter.

These values eventually are programmed into MMD registers.
There are 8 for 128-bit value.  Bit 0 in the last register is byte 0.
I do not see PHY register values are checked for endianness.

> > +             u8 pattern[42] = {
> > +                     0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF,
> > +                     0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> > +                     0x08, 0x06,
> > +                     0x00, 0x01, 0x08, 0x00, 0x06, 0x04, 0x00, 0x01,
> > +                     0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> > +                     0x00, 0x00, 0x00, 0x00,
> > +                     0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> > +                     0x00, 0x00, 0x00, 0x00 };
> 
> 
> > +     if (wol->wolopts & WAKE_MCAST) {
> > +             u8 pattern[6] = { 0x33, 0x33, 0xFF, 0x00, 0x00, 0x00 };
> > +             u16 mask[1] = { 0x0007 };
> > +             u8 len = 3;
> > +
> > +             /* Try to match IPv6 Neighbor Solicitation. */
> > +             if (ndev->ip6_ptr) {
> > +                     struct list_head *addr_list =
> > +                             &ndev->ip6_ptr->addr_list;
> > +                     struct inet6_ifaddr *ifa;
> > +
> > +                     list_for_each_entry(ifa, addr_list, if_list) {
> > +                             if (ifa->scope == IFA_LINK) {
> > +                                     memcpy(&pattern[3],
> > +                                            &ifa->addr.in6_u.u6_addr8[13],
> > +                                            3);
> > +                                     mask[0] = 0x003F;
> > +                                     len = 6;
> > +                                     break;
> > +                             }
> > +                     }
> > +             }
> 
> From an architecture point of view, i don't think a PHY driver should
> be access these data structure directly. See if ipv6_get_lladdr() does
> what you need?

ipv6_get_lladdr() is not exported so cannot be used when building the
PHY driver as a module.
The WAKE_ARP and WAKE_MCAST code just want a regular IPv4 address
and an IPv6 address as shown by ifconfig command.
They are more like an example to show how the hardware pattern filtering
is done.

> > +     if (wol->wolopts & (WAKE_MAGIC | WAKE_UCAST)) {
> > +             const u8 *mac = (const u8 *)ndev->dev_addr;
> > +
> > +             if (!is_valid_ether_addr(mac))
> > +                     return -EINVAL;
>
> Is that possible? Does the hardware care?
>

Removed.