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]

Message-ID: <20190124144622.Horde.QnDT4ajOrTiRSd1gwcXtjvR@www.vdorst.com>
Date:   Thu, 24 Jan 2019 14:46:22 +0000
From:   René van Dorst <opensource@...rst.com>
To:     Andrew Lunn <andrew@...n.ch>
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.

Quoting Andrew Lunn <andrew@...n.ch>:

>> >>+	/* 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.

Hi Andrew,

This can't happen, while loop is only executed when len > max_read_len.
len is reduced after a successful read in the while loop.

So sizes <= max_read_len is handled by sfp->read below the while loop.

>>
>> >>+		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

i2c-core already reports an error in i2c_check_for_quirks [0].
Is that sufficient?
This is how I know that something was wrong.
But the driver should not retry it for infinitely like what is  
happening with read.
And keeps spamming the error logs.

[0]  
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/i2c/i2c-core-base.c?h=v5.0-rc3#n1787

Greats,

René