[PATCH] net: sh_eth: avoid NULL pointer dereference in ring setup

[PATCH] net: sh_eth: avoid NULL pointer dereference in ring setup

[Wolfram Sang]

From: Wolfram Sang <wsa+renesas@sang-engineering.com>

When allocating an skb fails, rxdesc is still NULL (or the previous ring
index on further iterations of the loop). However, this pointer is
dereferenced after the loop. So, make sure rxdesc is updated immediately
at the beginning of the loop.

Reported-by: coverity (CID 1056464)
Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
 drivers/net/ethernet/renesas/sh_eth.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/renesas/sh_eth.c b/drivers/net/ethernet/renesas/sh_eth.c
index a2767336b7c545..d5f13d54099734 100644
--- a/drivers/net/ethernet/renesas/sh_eth.c
+++ b/drivers/net/ethernet/renesas/sh_eth.c
@@ -1120,6 +1120,7 @@ static void sh_eth_ring_format(struct net_device *ndev)
 
 	/* build Rx ring buffer */
 	for (i = 0; i < mdp->num_rx_ring; i++) {
+		rxdesc = &mdp->rx_ring[i];
 		/* skb */
 		mdp->rx_skbuff[i] = NULL;
 		skb = netdev_alloc_skb(ndev, skbuff_size);
@@ -1128,7 +1129,6 @@ static void sh_eth_ring_format(struct net_device *ndev)
 		sh_eth_set_receive_align(skb);
 
 		/* RX descriptor */
-		rxdesc = &mdp->rx_ring[i];
 		/* The size of the buffer is a multiple of 32 bytes. */
 		buf_len = ALIGN(mdp->rx_buf_sz, 32);
 		rxdesc->len = cpu_to_le32(buf_len << 16);
-- 
2.7.0

Re: [PATCH] net: sh_eth: avoid NULL pointer dereference in ring setup

[Sergei Shtylyov]

Hello.

On 03/02/2016 06:26 PM, Wolfram Sang wrote:

> From: Wolfram Sang <wsa+renesas@sang-engineering.com>
>
> When allocating an skb fails, rxdesc is still NULL (or the previous ring
> index on further iterations of the loop). However, this pointer is
> dereferenced after the loop.

    This is intended. What we seem to actually need is a NULL check before 
that dereference.

> So, make sure rxdesc is updated immediately
> at the beginning of the loop.

    No, this seems wrong. We don't want an unfilled descriptor to be marked as 
last, we still need the previous one marked as last. Actually, 'rxdesc' 
shouldn't be "advanced" even before the second *break* as it is now.

> Reported-by: coverity (CID 1056464)
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>

    Will you respin or should I?

MBR, Sergei

Re: [PATCH] net: sh_eth: avoid NULL pointer dereference in ring setup

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 299 bytes --]

> >Reported-by: coverity (CID 1056464)
> >Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> 
>    Will you respin or should I?

Please go ahead. You have more knowledge about this driver, so you will
be faster.

Reported-by: Wolfram Sang <wsa+renesas@sang-engineering.com>


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH] net: sh_eth: avoid NULL pointer dereference in ring setup

[Sergei Shtylyov]

On 03/02/2016 08:11 PM, Wolfram Sang wrote:

>>> Reported-by: coverity (CID 1056464)
>>> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
>>
>>     Will you respin or should I?
>
> Please go ahead. You have more knowledge about this driver, so you will
> be faster.

    Not necessarily -- I have other bug to chase right now. :-)

> Reported-by: Wolfram Sang <wsa+renesas@sang-engineering.com>

    Thanks for reporting, anyway.

MBR, Sergei