netdev - Re: [PATCH net-next 2/3] ionic: support ethtool get_module_eeprom_by

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: <891f1b66-0b49-41ce-bb00-5345ef9afb5c@lunn.ch>
Date: Tue, 15 Apr 2025 00:18:30 +0200
From: Andrew Lunn <andrew@...n.ch>
To: "Nelson, Shannon" <shannon.nelson@....com>
Cc: andrew+netdev@...n.ch, davem@...emloft.net, edumazet@...gle.com,
	kuba@...nel.org, pabeni@...hat.com, netdev@...r.kernel.org,
	linux-kernel@...r.kernel.org, brett.creeley@....com
Subject: Re: [PATCH net-next 2/3] ionic: support ethtool
 get_module_eeprom_by_page

> > > +static int ionic_get_module_eeprom_by_page(struct net_device *netdev,
> > > +                                        const struct ethtool_module_eeprom *page_data,
> > > +                                        struct netlink_ext_ack *extack)

> > > +     switch (page_data->page) {
> > > +     case 0:
> > > +             src = &idev->port_info->status.xcvr.sprom[page_data->offset];
> > > +             break;
> > > +     case 1:
> > > +             src = &idev->port_info->sprom_page1[page_data->offset - 128];
> > > +             break;
> > > +     case 2:
> > > +             src = &idev->port_info->sprom_page2[page_data->offset - 128];
> > > +             break;
> > > +     default:
> > > +             return -EINVAL;
> > 
> > It is a valid page, your firmware just does not support it. EOPNOSUPP.
> 
> I can see the argument for this, but EINVAL seems to me to match the
> out-of-bounds case from ionic_get_module_eprom(), as well as what other
> drivers are reporting for an unsupported address.  It seems to me that
> passing EOPNOSUPP back to the user is telling them that they can't get
> eeprom data at all, not that they asked for the wrong page.

I would disagree with at. Look at the ethtool usage:

ethtool -m|--dump-module-eeprom|--module-info devname [raw on|off]
          [hex on|off] [offset N] [length N] [page N] [bank N] [i2c N]

You can ask for any page. The only validation that can be done is,
does the page number fit within the page selection register. And that
is a u8. So any value < 256 is valid for page.  Some pages are
currently defined, some pages are reserved, and pages 128-255 are for
vendor specific functions.

The limitation here is your firmware, you don't support what the
specification allows. So EOPNOTSUPP for a page you don't supports
would give an indication of this.

ethtool's pretty print should handle -EOPNOTSUPP. It knows some netdev
have limits, and don't give full access to the module data. I would
not be too surprised to find ethtool actually interprets EINVAL for a
valid page to be fatal, but i've not checked. EOPNOTSUPP should just
stop it pretty printing that section of the module.

	Andrew