diff mbox series

[U-Boot,v2,06/13] net: ravb: Fix signed shift overflow

Message ID 20180826231332.2491-7-erosca@de.adit-jv.com
State Changes Requested
Delegated to: Tom Rini
Headers show
Series Import Undefined Behavior Sanitizer | expand

Commit Message

Eugeniu Rosca Aug. 26, 2018, 11:13 p.m. UTC 
Running "tftp" on R-Car H3 Salvator-X with CONFIG_UBSAN=y results in:

=> tftp
 =====================================================================
 UBSAN: Undefined behaviour in drivers/net/ravb.c:237:28
 left shift of 10 by 28 places cannot be represented in type 'int'
 =====================================================================
 =====================================================================
 UBSAN: Undefined behaviour in drivers/net/ravb.c:258:44
 left shift of 9 by 28 places cannot be represented in type 'int'
 =====================================================================
 =====================================================================
 UBSAN: Undefined behaviour in drivers/net/ravb.c:263:46
 left shift of 9 by 28 places cannot be represented in type 'int'
 =====================================================================
 =====================================================================
 UBSAN: Undefined behaviour in drivers/net/ravb.c:283:31
 left shift of 9 by 28 places cannot be represented in type 'int'
 =====================================================================
 =====================================================================
 UBSAN: Undefined behaviour in drivers/net/ravb.c:288:49
 left shift of 9 by 28 places cannot be represented in type 'int'
 =====================================================================
 =====================================================================
 UBSAN: Undefined behaviour in drivers/net/ravb.c:293:46
 left shift of 9 by 28 places cannot be represented in type 'int'
 =====================================================================
 =====================================================================
 UBSAN: Undefined behaviour in drivers/net/ravb.c:345:2
 left shift of 222 by 24 places cannot be represented in type 'int'
 =====================================================================

Pinging the host results in:

=> ping 192.168.2.11
 Using ethernet@e6800000 device
 =====================================================================
 UBSAN: Undefined behaviour in drivers/net/ravb.c:161:21
 left shift of 15 by 28 places cannot be represented in type 'int'
 =====================================================================
 =====================================================================
 UBSAN: Undefined behaviour in drivers/net/ravb.c:182:25
 left shift of 15 by 28 places cannot be represented in type 'int'
 =====================================================================
 =====================================================================
 UBSAN: Undefined behaviour in drivers/net/ravb.c:182:47
 left shift of 12 by 28 places cannot be represented in type 'int'
 =====================================================================
 =====================================================================
 UBSAN: Undefined behaviour in drivers/net/ravb.c:205:20
 left shift of 12 by 28 places cannot be represented in type 'int'
 =====================================================================
 host 192.168.2.11 is alive

There are two issues behind:
 - calculating RAVB_DESC_DT_* bitfields
 - assembling MAC address from its char components

Fix both.

Fixes: 8ae51b6f324e ("net: ravb: Add Renesas Ethernet RAVB driver")
Signed-off-by: Eugeniu Rosca <erosca@de.adit-jv.com>
Acked-by: Marek Vasut <marek.vasut@gmail.com>
---

Changes in v2:
 - Shorten the summary line
 - Add "Acked-by: Marek Vasut <marek.vasut@gmail.com>"
---
 drivers/net/ravb.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

Comments

Marek Vasut Aug. 26, 2018, 11:22 p.m. UTC | #1 
On 08/27/2018 01:13 AM, Eugeniu Rosca wrote:
> Running "tftp" on R-Car H3 Salvator-X with CONFIG_UBSAN=y results in:
> 
> => tftp
>  =====================================================================
>  UBSAN: Undefined behaviour in drivers/net/ravb.c:237:28
>  left shift of 10 by 28 places cannot be represented in type 'int'
>  =====================================================================
>  =====================================================================
>  UBSAN: Undefined behaviour in drivers/net/ravb.c:258:44
>  left shift of 9 by 28 places cannot be represented in type 'int'
>  =====================================================================
>  =====================================================================
>  UBSAN: Undefined behaviour in drivers/net/ravb.c:263:46
>  left shift of 9 by 28 places cannot be represented in type 'int'
>  =====================================================================
>  =====================================================================
>  UBSAN: Undefined behaviour in drivers/net/ravb.c:283:31
>  left shift of 9 by 28 places cannot be represented in type 'int'
>  =====================================================================
>  =====================================================================
>  UBSAN: Undefined behaviour in drivers/net/ravb.c:288:49
>  left shift of 9 by 28 places cannot be represented in type 'int'
>  =====================================================================
>  =====================================================================
>  UBSAN: Undefined behaviour in drivers/net/ravb.c:293:46
>  left shift of 9 by 28 places cannot be represented in type 'int'
>  =====================================================================
>  =====================================================================
>  UBSAN: Undefined behaviour in drivers/net/ravb.c:345:2
>  left shift of 222 by 24 places cannot be represented in type 'int'
>  =====================================================================
> 
> Pinging the host results in:
> 
> => ping 192.168.2.11
>  Using ethernet@e6800000 device
>  =====================================================================
>  UBSAN: Undefined behaviour in drivers/net/ravb.c:161:21
>  left shift of 15 by 28 places cannot be represented in type 'int'
>  =====================================================================
>  =====================================================================
>  UBSAN: Undefined behaviour in drivers/net/ravb.c:182:25
>  left shift of 15 by 28 places cannot be represented in type 'int'
>  =====================================================================
>  =====================================================================
>  UBSAN: Undefined behaviour in drivers/net/ravb.c:182:47
>  left shift of 12 by 28 places cannot be represented in type 'int'
>  =====================================================================
>  =====================================================================
>  UBSAN: Undefined behaviour in drivers/net/ravb.c:205:20
>  left shift of 12 by 28 places cannot be represented in type 'int'
>  =====================================================================
>  host 192.168.2.11 is alive
> 
> There are two issues behind:
>  - calculating RAVB_DESC_DT_* bitfields
>  - assembling MAC address from its char components
> 
> Fix both.
> 
> Fixes: 8ae51b6f324e ("net: ravb: Add Renesas Ethernet RAVB driver")
> Signed-off-by: Eugeniu Rosca <erosca@de.adit-jv.com>
> Acked-by: Marek Vasut <marek.vasut@gmail.com>
> ---
> 
> Changes in v2:
>  - Shorten the summary line
>  - Add "Acked-by: Marek Vasut <marek.vasut@gmail.com>"
> ---
>  drivers/net/ravb.c | 16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/net/ravb.c b/drivers/net/ravb.c
> index 749562db960e..5baff198889b 100644
> --- a/drivers/net/ravb.c
> +++ b/drivers/net/ravb.c
> @@ -73,12 +73,12 @@
>  #define RAVB_RX_QUEUE_OFFSET		4
>  
>  #define RAVB_DESC_DT(n)			((n) << 28)

What about changing this instead, ((u32)(n) << 28) ?

> -#define RAVB_DESC_DT_FSINGLE		RAVB_DESC_DT(0x7)
> -#define RAVB_DESC_DT_LINKFIX		RAVB_DESC_DT(0x9)
> -#define RAVB_DESC_DT_EOS		RAVB_DESC_DT(0xa)
> -#define RAVB_DESC_DT_FEMPTY		RAVB_DESC_DT(0xc)
> -#define RAVB_DESC_DT_EEMPTY		RAVB_DESC_DT(0x3)
> -#define RAVB_DESC_DT_MASK		RAVB_DESC_DT(0xf)
> +#define RAVB_DESC_DT_FSINGLE		RAVB_DESC_DT(0x7UL)
> +#define RAVB_DESC_DT_LINKFIX		RAVB_DESC_DT(0x9UL)
> +#define RAVB_DESC_DT_EOS		RAVB_DESC_DT(0xaUL)
> +#define RAVB_DESC_DT_FEMPTY		RAVB_DESC_DT(0xcUL)
> +#define RAVB_DESC_DT_EEMPTY		RAVB_DESC_DT(0x3UL)
> +#define RAVB_DESC_DT_MASK		RAVB_DESC_DT(0xfUL)
>  
>  #define RAVB_DESC_DS(n)			(((n) & 0xfff) << 0)
>  #define RAVB_DESC_DS_MASK		0xfff
> @@ -342,8 +342,8 @@ static int ravb_write_hwaddr(struct udevice *dev)
>  	struct eth_pdata *pdata = dev_get_platdata(dev);
>  	unsigned char *mac = pdata->enetaddr;
>  
> -	writel((mac[0] << 24) | (mac[1] << 16) | (mac[2] << 8) | mac[3],
> -	       eth->iobase + RAVB_REG_MAHR);
> +	writel(((u32)mac[0] << 24) | ((u32)mac[1] << 16) | ((u32)mac[2] << 8) |
> +	       mac[3], eth->iobase + RAVB_REG_MAHR);

Not a big fan of the casts here, I wonder if there isn't some more
elegant solution. If not, so be it.

>  	writel((mac[4] << 8) | mac[5], eth->iobase + RAVB_REG_MALR);
>  
>
Eugeniu Rosca Aug. 27, 2018, 8:24 p.m. UTC | #2 
Hi Marek,

On Mon, Aug 27, 2018 at 01:22:54AM +0200, Marek Vasut wrote:
> On 08/27/2018 01:13 AM, Eugeniu Rosca wrote:
[...]
> >  
> >  #define RAVB_DESC_DT(n)			((n) << 28)
> 
> What about changing this instead, ((u32)(n) << 28) ?

This works too.

[...]

> >  
> > -	writel((mac[0] << 24) | (mac[1] << 16) | (mac[2] << 8) | mac[3],
> > -	       eth->iobase + RAVB_REG_MAHR);
> > +	writel(((u32)mac[0] << 24) | ((u32)mac[1] << 16) | ((u32)mac[2] << 8) |
> > +	       mac[3], eth->iobase + RAVB_REG_MAHR);
> 
> Not a big fan of the casts here, I wonder if there isn't some more
> elegant solution. If not, so be it.

Actually one cast is enough to fix the UB.
Let me know if below patch looks better to you.

diff --git a/drivers/net/ravb.c b/drivers/net/ravb.c
index 749562db960e..2190811c53bb 100644
--- a/drivers/net/ravb.c
+++ b/drivers/net/ravb.c
@@ -72,7 +72,7 @@
 #define RAVB_TX_QUEUE_OFFSET		0
 #define RAVB_RX_QUEUE_OFFSET		4
 
-#define RAVB_DESC_DT(n)			((n) << 28)
+#define RAVB_DESC_DT(n)			((u32)(n) << 28)
 #define RAVB_DESC_DT_FSINGLE		RAVB_DESC_DT(0x7)
 #define RAVB_DESC_DT_LINKFIX		RAVB_DESC_DT(0x9)
 #define RAVB_DESC_DT_EOS		RAVB_DESC_DT(0xa)
@@ -342,7 +342,7 @@ static int ravb_write_hwaddr(struct udevice *dev)
 	struct eth_pdata *pdata = dev_get_platdata(dev);
 	unsigned char *mac = pdata->enetaddr;
 
-	writel((mac[0] << 24) | (mac[1] << 16) | (mac[2] << 8) | mac[3],
+	writel(((u32)mac[0] << 24) | (mac[1] << 16) | (mac[2] << 8) | mac[3],
 	       eth->iobase + RAVB_REG_MAHR);
 
 	writel((mac[4] << 8) | mac[5], eth->iobase + RAVB_REG_MALR);


Thanks,
Eugeniu.
Marek Vasut Aug. 27, 2018, 11:55 p.m. UTC | #3 
On 08/27/2018 10:24 PM, Eugeniu Rosca wrote:
> Hi Marek,

Hi,

> On Mon, Aug 27, 2018 at 01:22:54AM +0200, Marek Vasut wrote:
>> On 08/27/2018 01:13 AM, Eugeniu Rosca wrote:
> [...]
>>>  
>>>  #define RAVB_DESC_DT(n)			((n) << 28)
>>
>> What about changing this instead, ((u32)(n) << 28) ?
> 
> This works too.
> 
> [...]
> 
>>>  
>>> -	writel((mac[0] << 24) | (mac[1] << 16) | (mac[2] << 8) | mac[3],
>>> -	       eth->iobase + RAVB_REG_MAHR);
>>> +	writel(((u32)mac[0] << 24) | ((u32)mac[1] << 16) | ((u32)mac[2] << 8) |
>>> +	       mac[3], eth->iobase + RAVB_REG_MAHR);
>>
>> Not a big fan of the casts here, I wonder if there isn't some more
>> elegant solution. If not, so be it.
> 
> Actually one cast is enough to fix the UB.
> Let me know if below patch looks better to you.

I guess, it's less intrusive. What do you think ?

> diff --git a/drivers/net/ravb.c b/drivers/net/ravb.c
> index 749562db960e..2190811c53bb 100644
> --- a/drivers/net/ravb.c
> +++ b/drivers/net/ravb.c
> @@ -72,7 +72,7 @@
>  #define RAVB_TX_QUEUE_OFFSET		0
>  #define RAVB_RX_QUEUE_OFFSET		4
>  
> -#define RAVB_DESC_DT(n)			((n) << 28)
> +#define RAVB_DESC_DT(n)			((u32)(n) << 28)
>  #define RAVB_DESC_DT_FSINGLE		RAVB_DESC_DT(0x7)
>  #define RAVB_DESC_DT_LINKFIX		RAVB_DESC_DT(0x9)
>  #define RAVB_DESC_DT_EOS		RAVB_DESC_DT(0xa)
> @@ -342,7 +342,7 @@ static int ravb_write_hwaddr(struct udevice *dev)
>  	struct eth_pdata *pdata = dev_get_platdata(dev);
>  	unsigned char *mac = pdata->enetaddr;
>  
> -	writel((mac[0] << 24) | (mac[1] << 16) | (mac[2] << 8) | mac[3],
> +	writel(((u32)mac[0] << 24) | (mac[1] << 16) | (mac[2] << 8) | mac[3],
>  	       eth->iobase + RAVB_REG_MAHR);
>  
>  	writel((mac[4] << 8) | mac[5], eth->iobase + RAVB_REG_MALR);
> 
> 
> Thanks,
> Eugeniu.
>
diff mbox series

Patch

diff --git a/drivers/net/ravb.c b/drivers/net/ravb.c
index 749562db960e..5baff198889b 100644
--- a/drivers/net/ravb.c
+++ b/drivers/net/ravb.c
@@ -73,12 +73,12 @@ 
 #define RAVB_RX_QUEUE_OFFSET		4
 
 #define RAVB_DESC_DT(n)			((n) << 28)
-#define RAVB_DESC_DT_FSINGLE		RAVB_DESC_DT(0x7)
-#define RAVB_DESC_DT_LINKFIX		RAVB_DESC_DT(0x9)
-#define RAVB_DESC_DT_EOS		RAVB_DESC_DT(0xa)
-#define RAVB_DESC_DT_FEMPTY		RAVB_DESC_DT(0xc)
-#define RAVB_DESC_DT_EEMPTY		RAVB_DESC_DT(0x3)
-#define RAVB_DESC_DT_MASK		RAVB_DESC_DT(0xf)
+#define RAVB_DESC_DT_FSINGLE		RAVB_DESC_DT(0x7UL)
+#define RAVB_DESC_DT_LINKFIX		RAVB_DESC_DT(0x9UL)
+#define RAVB_DESC_DT_EOS		RAVB_DESC_DT(0xaUL)
+#define RAVB_DESC_DT_FEMPTY		RAVB_DESC_DT(0xcUL)
+#define RAVB_DESC_DT_EEMPTY		RAVB_DESC_DT(0x3UL)
+#define RAVB_DESC_DT_MASK		RAVB_DESC_DT(0xfUL)
 
 #define RAVB_DESC_DS(n)			(((n) & 0xfff) << 0)
 #define RAVB_DESC_DS_MASK		0xfff
@@ -342,8 +342,8 @@  static int ravb_write_hwaddr(struct udevice *dev)
 	struct eth_pdata *pdata = dev_get_platdata(dev);
 	unsigned char *mac = pdata->enetaddr;
 
-	writel((mac[0] << 24) | (mac[1] << 16) | (mac[2] << 8) | mac[3],
-	       eth->iobase + RAVB_REG_MAHR);
+	writel(((u32)mac[0] << 24) | ((u32)mac[1] << 16) | ((u32)mac[2] << 8) |
+	       mac[3], eth->iobase + RAVB_REG_MAHR);
 
 	writel((mac[4] << 8) | mac[5], eth->iobase + RAVB_REG_MALR);