netdev - RE: [PATCH v1 net-next] lan743x: Provide Read/Write Access to on chip OTP

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:   Mon, 21 Jan 2019 21:12:41 +0000
From:   <Bryan.Whitehead@...rochip.com>
To:     <andrew@...n.ch>
CC:     <davem@...emloft.net>, <netdev@...r.kernel.org>,
        <UNGLinuxDriver@...rochip.com>
Subject: RE: [PATCH v1 net-next] lan743x: Provide Read/Write Access to on chip
 OTP

> > +static int lan743x_otp_write(struct lan743x_adapter *adapter, u32 offset,
> > +			     u32 length, u8 *data)
> > +{
> > +	int ret;
> > +	int i;
> > +
> > +	ret = lan743x_otp_power_up(adapter);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	ret = lan743x_otp_wait_till_not_busy(adapter);
> > +	if (ret < 0)
> > +		return ret;
> > +
> >  	/* set to BYTE program mode */
> >  	lan743x_csr_write(adapter, OTP_PRGM_MODE,
> OTP_PRGM_MODE_BYTE_);
> >
> >  	for (i = 0; i < length; i++) {
> 
> I think you now need to verify length. It might be possible to change from
> EEPROM to OTP after lan743x_ethtool_get_eeprom_len() has been called,
> so you have been asked to write EEPROM length into the OTP.
> The same is also true for read.

OK, will do.

> >  static int lan743x_ethtool_set_eeprom(struct net_device *netdev, @@
> > -224,17 +306,18 @@ static int lan743x_ethtool_set_eeprom(struct
> net_device *netdev,
> >  	struct lan743x_adapter *adapter = netdev_priv(netdev);
> >  	int ret = -EINVAL;
> >
> > -	if (ee->magic == LAN743X_EEPROM_MAGIC)
> > -		ret = lan743x_eeprom_write(adapter, ee->offset, ee->len,
> > -					   data);
> > -	/* Beware!  OTP is One Time Programming ONLY!
> > -	 * So do some strict condition check before messing up
> > -	 */
> > -	else if ((ee->magic == LAN743X_OTP_MAGIC) &&
> > -		 (ee->offset == 0) &&
> > -		 (ee->len == MAX_EEPROM_SIZE) &&
> > -		 (data[0] == OTP_INDICATOR_1))
> > -		ret = lan743x_otp_write(adapter, ee->offset, ee->len, data);
> > +	if (adapter->flags & LAN743X_ADAPTER_FLAG_OTP) {
> > +		/* Beware!  OTP is One Time Programming ONLY! */
> > +		if (ee->magic == LAN743X_OTP_MAGIC) {
> > +			ret = lan743x_otp_write(adapter, ee->offset,
> > +						ee->len, data);
> > +		}
> > +	} else {
> > +		if (ee->magic == LAN743X_EEPROM_MAGIC) {
> > +			ret = lan743x_eeprom_write(adapter, ee->offset,
> > +						   ee->len, data);
> > +		}
> > +	}
> 
> This is breaking backwards compatibility. I think you need to respect the
> magic value, independent of how adapter->flags.

Is backwards compatibility a requirement?

If so, this only breaks OTP backward compatibility, which was extremely
  restrictive and ultimately not very useful.
In exchange for looser restrictions, we believe it is a good
  idea to make the user take the extra step to set the OTP_ACCESS flag
  to true. The reason for this is the OTP is One Time Programming only,
  and we want to make sure the user really intends to program it.

Since the OTP_ACCESS/LAN743X_ADAPTER_FLAG_OTP is false/0 by default,
  previously used EEPROM programming commands will work exactly the same.

We realize the magic value is also intended to "make sure the user really
  intends to program it".
But the magic value is not available for the read command, or length reporting.
And we believe it is easier for the user to understand the mechanics
  when one flag is used to control reading, writing, and length reporting.
If I did not respect the OTP_ACCESS flag for writing, then the length reporting
  could be invalid for writing operations.

Do you agree?