r8169: fix invalid register dump

On Wednesday 14 August 2013 21:58:29 Francois Romieu wrote:
> > -     memcpy_fromio(p, tp->mmio_addr, regs->len);
> > +     if (regs->len >= 4) {
> > +             for (i = 0; i < regs->len - 4; i += 4)
> > +                     memcpy_fromio(bytes + i, tp->mmio_addr + i, 4);
> > +     }
> > +     if (i < regs->len)
> 
> Comparison with random stack stuff when regs->len < 4. :o/

Right, let's rm $OLD_PATCH and consider this one.

Checklist:
1. super large regs->len: won't be greater than R8169_REGS_SIZE (256)
2. regs->len == 0: 0 < 0 is false, nothing is copied
3. regs->len is 1, 2 or 3: i = 0, at most 3 bytes will be copied
4. regs->len is 4, i < 4 - 4, skip loop, 0 < regs->len, copy 4
5. regs->len is 5, i < 5 - 4, copy; 4 < regs->len, copy 1

With this I can now say with confidence that I haven't overlooked something
related to integer overflow. You have a very sharp eye, thanks for
catching my mistakes.

Regards,
Peter
---
From: Peter Wu <lekensteyn@gmail.com>

For some reason, my PCIe RTL8111E onboard NIC on a GA-Z68X-UD3H-B3
motherboard reads as FFs when reading from MMIO with a block size
larger than 7. Therefore change to reading blocks of four bytes.

Signed-off-by: Peter Wu <lekensteyn@gmail.com>
---
 drivers/net/ethernet/realtek/r8169.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

Peter Wu <lekensteyn@gmail.com> :
[...]
> Checklist:
> 1. super large regs->len: won't be greater than R8169_REGS_SIZE (256)
> 2. regs->len == 0: 0 < 0 is false, nothing is copied
> 3. regs->len is 1, 2 or 3: i = 0, at most 3 bytes will be copied
> 4. regs->len is 4, i < 4 - 4, skip loop, 0 < regs->len, copy 4
> 5. regs->len is 5, i < 5 - 4, copy; 4 < regs->len, copy 1

Spartan implementation:

	u32 __iomem *ioaddr = tp->mmio_addr;
	u32 *dw = p;
	int i;

	...

	for (i = regs->len; i > 0; i -= 4)
		memcpy_fromio(dw++, ioaddr++, min(i, 4));

or (min() verges on gluttony):

	for (i = regs->len; i >= 4; i -= 4)
		memcpy_fromio(dw++, ioaddr++, 4);

	if (i > 0)
		memcpy_fromio(dw, ioaddr, i);

On Wed, 2013-08-14 at 23:31 +0200, Peter Wu wrote:
> On Wednesday 14 August 2013 21:58:29 Francois Romieu wrote:
> > > -     memcpy_fromio(p, tp->mmio_addr, regs->len);
> > > +     if (regs->len >= 4) {
> > > +             for (i = 0; i < regs->len - 4; i += 4)
> > > +                     memcpy_fromio(bytes + i, tp->mmio_addr + i, 4);
> > > +     }
> > > +     if (i < regs->len)
> > 
> > Comparison with random stack stuff when regs->len < 4. :o/
> 
> Right, let's rm $OLD_PATCH and consider this one.
> 
> Checklist:
> 1. super large regs->len: won't be greater than R8169_REGS_SIZE (256)
> 2. regs->len == 0: 0 < 0 is false, nothing is copied
> 3. regs->len is 1, 2 or 3: i = 0, at most 3 bytes will be copied
> 4. regs->len is 4, i < 4 - 4, skip loop, 0 < regs->len, copy 4
> 5. regs->len is 5, i < 5 - 4, copy; 4 < regs->len, copy 1
[...]

The kernel buffer size is max(regs->len,
dev->ethtool_ops->get_regs_len()).  So you can safely ignore regs->len
and always read all your registers.

Ben.

On Thursday 15 August 2013 22:33:15 Ben Hutchings wrote:
> The kernel buffer size is max(regs->len,
> dev->ethtool_ops->get_regs_len()).  So you can safely ignore regs->len
> and always read all your registers.

I see, that is something for a different patch though. While I am at it,
I checked all users of get_regs_len in drivers/net and found only one
other user that also checked the length. Patch will follow shortly.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html