netdev - Re: [PATCH] sfp: sfp_read: split-up request when hw rx buffer is too small.

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:   Thu, 24 Jan 2019 15:12:01 +0100
From:   Andrew Lunn <andrew@...n.ch>
To:     René van Dorst <opensource@...rst.com>
Cc:     Florian Fainelli <f.fainelli@...il.com>,
        Heiner Kallweit <hkallweit1@...il.com>,
        "David S. Miller" <davem@...emloft.net>, netdev@...r.kernel.org
Subject: Re: [PATCH] sfp: sfp_read: split-up request when hw rx buffer is too
 small.

> >>+	/* Many i2c hw have limited rx buffers, split-up request when needed. */
> >>+	while ((q->max_read_len) && (len > q->max_read_len)) {
> >>+		ret = sfp->read(sfp, a2, addr, buf, q->max_read_len);
> >
> >Hi René
> >
> >I think you want to pass MIN(len, q->max_read_len) to read().
> 
> Hi Andrew,
> 
> max_read_len is 0 when there is no quirk.
> I can write it a bit differently depending on the outcome of my other email.

Hi René

No, you misunderstood me.

> >>+		ret = sfp->read(sfp, a2, addr, buf, q->max_read_len);

Say max_read_len = 64

The SFP code asks to read 68 bytes. The first call to read() is going
to ask for 64 bytes. The second call is going to also ask for 64
bytes, writing 60 bytes passed the end of buf. Bad things then happen.

> 
> >>+		if (ret < 0)
> >>+			return ret;
> >>+		rx_bytes += ret;
> >>+		addr += q->max_read_len;
> >>+		buf += q->max_read_len;
> >>+		len -= q->max_read_len;
> >
> >I would prefer you add ret, not q->max_read_len. There is a danger it
> >returned less bytes than you asked for.
> 
> Getting less bytes then asked is already an error I think.
> I could check the return size and directly return the number of bytes that I
> have. The callers are checking for size and they can retry if wanted. So that
> should not be an issue.

If that is true, why is rx_bytes += ret, where as all the others are
+= q->max_read_len. Please be consistent. The general pattern of a
read function in POSIX systems is that it returns how many bytes were
actually returned. So i would always use += ret.

> By reading the SSF spec we can write to a user writable EERPOM area of 120
> bytes.
> But the current code has only has 1 sfp_write for a byte value.
> 
> So for now I should say no.

So how about adding a WARN_ON. If the request is bigger than what the
quirk allows, make it very obvious we have an issue.

      Andrew