[PATCH] net:fec: fixed unaligned access and stack corruption

[PATCH] net:fec: fixed unaligned access and stack corruption

[Enrico Scholz]

on 64 bit architectures, the 'enum fec_type' might not be aligned and
large enough to hold a pointer.  When running barebox without MMU,
this will crash like

| i.MX8MM unique ID: dab4b7491a2c4209
| DABT (current EL) exception (ESR 0x96000061) at 0x00000000fffefeb4
| elr: 00000000ffe14c28 lr : 00000000ffe196e0
| x0 : 0000000000000002 x1 : 00000000fffefeb4
| x2 : 00000000ffe91370 x3 : 00000000bfe1b6e8
| x4 : 0000000000000000 x5 : 0000000011000000
| ...
| Call trace:
| [<ffe14c28>] (dev_get_drvdata+0xc/0x30) from [<ffe1446c>] (device_probe+0x54/0xd0)
| [<ffe1446c>] (device_probe+0x54/0xd0) from [<ffe14530>] (match+0x48/0x58)
| [<ffe14530>] (match+0x48/0x58) from [<ffe14a64>] (register_driver+0xc0/0xd0)
| [<ffe14a64>] (register_driver+0xc0/0xd0) from [<ffe01738>] (start_barebox+0x64/0x90)

Signed-off-by: Enrico Scholz <enrico.scholz@sigma-chemnitz.de>
---
 drivers/net/fec_imx.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/net/fec_imx.c b/drivers/net/fec_imx.c
index 772f930f0d08..30ee7841faba 100644
--- a/drivers/net/fec_imx.c
+++ b/drivers/net/fec_imx.c
@@ -739,14 +739,17 @@ static int fec_probe(struct device_d *dev)
 	void *base;
 	int ret;
 	enum fec_type type;
+	void const *type_v;
 	int phy_reset;
 	u32 msec = 1, phy_post_delay = 0;
 	u32 reg;
 
-	ret = dev_get_drvdata(dev, (const void **)&type);
+	ret = dev_get_drvdata(dev, &type_v);
 	if (ret)
 		return ret;
 
+	type = (uintptr_t)(type_v);
+
 	fec = xzalloc(sizeof(*fec));
 	fec->type = type;
 	fec->dev = dev;
-- 
2.25.4


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH] net:fec: fixed unaligned access and stack corruption

[Ahmad Fatoum]

On 7/7/20 6:01 PM, Enrico Scholz wrote:
> on 64 bit architectures, the 'enum fec_type' might not be aligned and
> large enough to hold a pointer.  

I am wondering if we couldn't just adopt the Linux prototype:
void *dev_get_drvdata(const struct device_d *dev);

and do away with the error code and most of the casts.
Users won't be able to differentiate between NULL from id table
and NULL due to lack of drvdata, but I don't think this is
that much of a downside, compared with not having casts obscure
the more common pitfall (besides fec_imx.c, lm75.c, apbh_dma.c and nand_mxs.c
are affected as well of which probably only the first is an issue.)

@Sascha, thoughts?

> When running barebox without MMU,
> this will crash like
> 
> | i.MX8MM unique ID: dab4b7491a2c4209
> | DABT (current EL) exception (ESR 0x96000061) at 0x00000000fffefeb4
> | elr: 00000000ffe14c28 lr : 00000000ffe196e0
> | x0 : 0000000000000002 x1 : 00000000fffefeb4
> | x2 : 00000000ffe91370 x3 : 00000000bfe1b6e8
> | x4 : 0000000000000000 x5 : 0000000011000000
> | ...
> | Call trace:
> | [<ffe14c28>] (dev_get_drvdata+0xc/0x30) from [<ffe1446c>] (device_probe+0x54/0xd0)
> | [<ffe1446c>] (device_probe+0x54/0xd0) from [<ffe14530>] (match+0x48/0x58)
> | [<ffe14530>] (match+0x48/0x58) from [<ffe14a64>] (register_driver+0xc0/0xd0)
> | [<ffe14a64>] (register_driver+0xc0/0xd0) from [<ffe01738>] (start_barebox+0x64/0x90)
> 
> Signed-off-by: Enrico Scholz <enrico.scholz@sigma-chemnitz.de>
> ---
>  drivers/net/fec_imx.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/fec_imx.c b/drivers/net/fec_imx.c
> index 772f930f0d08..30ee7841faba 100644
> --- a/drivers/net/fec_imx.c
> +++ b/drivers/net/fec_imx.c
> @@ -739,14 +739,17 @@ static int fec_probe(struct device_d *dev)
>  	void *base;
>  	int ret;
>  	enum fec_type type;
> +	void const *type_v;
>  	int phy_reset;
>  	u32 msec = 1, phy_post_delay = 0;
>  	u32 reg;
>  
> -	ret = dev_get_drvdata(dev, (const void **)&type);
> +	ret = dev_get_drvdata(dev, &type_v);
>  	if (ret)
>  		return ret;
>  
> +	type = (uintptr_t)(type_v);
> +
>  	fec = xzalloc(sizeof(*fec));
>  	fec->type = type;
>  	fec->dev = dev;
> 

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH] net:fec: fixed unaligned access and stack corruption

[Sascha Hauer]

On Tue, Jul 07, 2020 at 07:11:31PM +0200, Ahmad Fatoum wrote:
> On 7/7/20 6:01 PM, Enrico Scholz wrote:
> > on 64 bit architectures, the 'enum fec_type' might not be aligned and
> > large enough to hold a pointer.  
> 
> I am wondering if we couldn't just adopt the Linux prototype:
> void *dev_get_drvdata(const struct device_d *dev);
> 
> and do away with the error code and most of the casts.
> Users won't be able to differentiate between NULL from id table
> and NULL due to lack of drvdata, but I don't think this is
> that much of a downside, compared with not having casts obscure
> the more common pitfall (besides fec_imx.c, lm75.c, apbh_dma.c and nand_mxs.c
> are affected as well of which probably only the first is an issue.)

Sounds good. When we change this we should rename the function
alongside, because dev_get_drvdata() does something different in Linux.

Sascha

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH] net:fec: fixed unaligned access and stack corruption

[Sascha Hauer]

On Tue, Jul 07, 2020 at 06:01:24PM +0200, Enrico Scholz wrote:
> on 64 bit architectures, the 'enum fec_type' might not be aligned and
> large enough to hold a pointer.  When running barebox without MMU,
> this will crash like
> 
> | i.MX8MM unique ID: dab4b7491a2c4209
> | DABT (current EL) exception (ESR 0x96000061) at 0x00000000fffefeb4
> | elr: 00000000ffe14c28 lr : 00000000ffe196e0
> | x0 : 0000000000000002 x1 : 00000000fffefeb4
> | x2 : 00000000ffe91370 x3 : 00000000bfe1b6e8
> | x4 : 0000000000000000 x5 : 0000000011000000
> | ...
> | Call trace:
> | [<ffe14c28>] (dev_get_drvdata+0xc/0x30) from [<ffe1446c>] (device_probe+0x54/0xd0)
> | [<ffe1446c>] (device_probe+0x54/0xd0) from [<ffe14530>] (match+0x48/0x58)
> | [<ffe14530>] (match+0x48/0x58) from [<ffe14a64>] (register_driver+0xc0/0xd0)
> | [<ffe14a64>] (register_driver+0xc0/0xd0) from [<ffe01738>] (start_barebox+0x64/0x90)
> 
> Signed-off-by: Enrico Scholz <enrico.scholz@sigma-chemnitz.de>
> ---
>  drivers/net/fec_imx.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)

Until Ahmads suggestion is in place I applied this as a stop gap
solution.

Sascha

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH] net:fec: fixed unaligned access and stack corruption

[Ahmad Fatoum]

On 7/11/20 7:07 AM, Sascha Hauer wrote:
> On Tue, Jul 07, 2020 at 07:11:31PM +0200, Ahmad Fatoum wrote:
>> On 7/7/20 6:01 PM, Enrico Scholz wrote:
>>> on 64 bit architectures, the 'enum fec_type' might not be aligned and
>>> large enough to hold a pointer.  
>>
>> I am wondering if we couldn't just adopt the Linux prototype:
>> void *dev_get_drvdata(const struct device_d *dev);
>>
>> and do away with the error code and most of the casts.
>> Users won't be able to differentiate between NULL from id table
>> and NULL due to lack of drvdata, but I don't think this is
>> that much of a downside, compared with not having casts obscure
>> the more common pitfall (besides fec_imx.c, lm75.c, apbh_dma.c and nand_mxs.c
>> are affected as well of which probably only the first is an issue.)
> 
> Sounds good. When we change this we should rename the function
> alongside, because dev_get_drvdata() does something different in Linux.

Does it? I thought it does the same, with the difference that it can
be set with dev_set_drvdata as well. At the cost of one extra pointer
per device_d, we could have both of them.

> 
> Sascha
> 

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH] net:fec: fixed unaligned access and stack corruption

[Sascha Hauer]

On Sat, Jul 11, 2020 at 07:13:11AM +0200, Ahmad Fatoum wrote:
> 
> 
> On 7/11/20 7:07 AM, Sascha Hauer wrote:
> > On Tue, Jul 07, 2020 at 07:11:31PM +0200, Ahmad Fatoum wrote:
> >> On 7/7/20 6:01 PM, Enrico Scholz wrote:
> >>> on 64 bit architectures, the 'enum fec_type' might not be aligned and
> >>> large enough to hold a pointer.  
> >>
> >> I am wondering if we couldn't just adopt the Linux prototype:
> >> void *dev_get_drvdata(const struct device_d *dev);
> >>
> >> and do away with the error code and most of the casts.
> >> Users won't be able to differentiate between NULL from id table
> >> and NULL due to lack of drvdata, but I don't think this is
> >> that much of a downside, compared with not having casts obscure
> >> the more common pitfall (besides fec_imx.c, lm75.c, apbh_dma.c and nand_mxs.c
> >> are affected as well of which probably only the first is an issue.)
> > 
> > Sounds good. When we change this we should rename the function
> > alongside, because dev_get_drvdata() does something different in Linux.
> 
> Does it? I thought it does the same, with the difference that it can
> be set with dev_set_drvdata as well. At the cost of one extra pointer
> per device_d, we could have both of them.

dev_set_drvdata() in Linux allows you to store a pointer to private
driver data in struct device. We don't have a function for that in
barebox and use dev->priv instead.
dev_get_drvdata() in barebox gets you the device type data (or however
we want to call it). There's no function for that in Linux and we have
to first find out if we probe from platform data or from device tree
to pick the right function to get the data.

Sascha

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH] net:fec: fixed unaligned access and stack corruption

[Ahmad Fatoum]

Hi,

On 7/11/20 7:20 AM, Sascha Hauer wrote:
> On Sat, Jul 11, 2020 at 07:13:11AM +0200, Ahmad Fatoum wrote:
>>
>>
>> On 7/11/20 7:07 AM, Sascha Hauer wrote:
>>> On Tue, Jul 07, 2020 at 07:11:31PM +0200, Ahmad Fatoum wrote:
>>>> On 7/7/20 6:01 PM, Enrico Scholz wrote:
>>>>> on 64 bit architectures, the 'enum fec_type' might not be aligned and
>>>>> large enough to hold a pointer.  
>>>>
>>>> I am wondering if we couldn't just adopt the Linux prototype:
>>>> void *dev_get_drvdata(const struct device_d *dev);
>>>>
>>>> and do away with the error code and most of the casts.
>>>> Users won't be able to differentiate between NULL from id table
>>>> and NULL due to lack of drvdata, but I don't think this is
>>>> that much of a downside, compared with not having casts obscure
>>>> the more common pitfall (besides fec_imx.c, lm75.c, apbh_dma.c and nand_mxs.c
>>>> are affected as well of which probably only the first is an issue.)
>>>
>>> Sounds good. When we change this we should rename the function
>>> alongside, because dev_get_drvdata() does something different in Linux.
>>
>> Does it? I thought it does the same, with the difference that it can
>> be set with dev_set_drvdata as well. At the cost of one extra pointer
>> per device_d, we could have both of them.
> 
> dev_set_drvdata() in Linux allows you to store a pointer to private
> driver data in struct device. We don't have a function for that in
> barebox and use dev->priv instead.
> dev_get_drvdata() in barebox gets you the device type data (or however
> we want to call it). There's no function for that in Linux and we have
> to first find out if we probe from platform data or from device tree
> to pick the right function to get the data.

Ah, I thought the drvdata is pre-populated in Linux. I would rename
the new function to device_get_match_data then for alignment with
Linux, with the difference that it returns either platform data or
device tree driver_data as appropriate.

Thanks
Ahmad

> 
> Sascha
> 

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH] net:fec: fixed unaligned access and stack corruption

[Sascha Hauer]

On Sat, Jul 11, 2020 at 07:28:58AM +0200, Ahmad Fatoum wrote:
> Hi,
> 
> On 7/11/20 7:20 AM, Sascha Hauer wrote:
> > On Sat, Jul 11, 2020 at 07:13:11AM +0200, Ahmad Fatoum wrote:
> >>
> >>
> >> On 7/11/20 7:07 AM, Sascha Hauer wrote:
> >>> On Tue, Jul 07, 2020 at 07:11:31PM +0200, Ahmad Fatoum wrote:
> >>>> On 7/7/20 6:01 PM, Enrico Scholz wrote:
> >>>>> on 64 bit architectures, the 'enum fec_type' might not be aligned and
> >>>>> large enough to hold a pointer.  
> >>>>
> >>>> I am wondering if we couldn't just adopt the Linux prototype:
> >>>> void *dev_get_drvdata(const struct device_d *dev);
> >>>>
> >>>> and do away with the error code and most of the casts.
> >>>> Users won't be able to differentiate between NULL from id table
> >>>> and NULL due to lack of drvdata, but I don't think this is
> >>>> that much of a downside, compared with not having casts obscure
> >>>> the more common pitfall (besides fec_imx.c, lm75.c, apbh_dma.c and nand_mxs.c
> >>>> are affected as well of which probably only the first is an issue.)
> >>>
> >>> Sounds good. When we change this we should rename the function
> >>> alongside, because dev_get_drvdata() does something different in Linux.
> >>
> >> Does it? I thought it does the same, with the difference that it can
> >> be set with dev_set_drvdata as well. At the cost of one extra pointer
> >> per device_d, we could have both of them.
> > 
> > dev_set_drvdata() in Linux allows you to store a pointer to private
> > driver data in struct device. We don't have a function for that in
> > barebox and use dev->priv instead.
> > dev_get_drvdata() in barebox gets you the device type data (or however
> > we want to call it). There's no function for that in Linux and we have
> > to first find out if we probe from platform data or from device tree
> > to pick the right function to get the data.
> 
> Ah, I thought the drvdata is pre-populated in Linux. I would rename
> the new function to device_get_match_data then for alignment with
> Linux, with the difference that it returns either platform data or
> device tree driver_data as appropriate.

Sounds good.

Sascha


-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH] net:fec: fixed unaligned access and stack corruption

[Marco Felsch]

Hi,

sorry for jumping in.

On 20-07-11 07:28, Ahmad Fatoum wrote:
> Hi,
> 
> On 7/11/20 7:20 AM, Sascha Hauer wrote:
> > On Sat, Jul 11, 2020 at 07:13:11AM +0200, Ahmad Fatoum wrote:
> >>
> >>
> >> On 7/11/20 7:07 AM, Sascha Hauer wrote:
> >>> On Tue, Jul 07, 2020 at 07:11:31PM +0200, Ahmad Fatoum wrote:
> >>>> On 7/7/20 6:01 PM, Enrico Scholz wrote:
> >>>>> on 64 bit architectures, the 'enum fec_type' might not be aligned and
> >>>>> large enough to hold a pointer.  
> >>>>
> >>>> I am wondering if we couldn't just adopt the Linux prototype:
> >>>> void *dev_get_drvdata(const struct device_d *dev);
> >>>>
> >>>> and do away with the error code and most of the casts.
> >>>> Users won't be able to differentiate between NULL from id table
> >>>> and NULL due to lack of drvdata, but I don't think this is
> >>>> that much of a downside, compared with not having casts obscure
> >>>> the more common pitfall (besides fec_imx.c, lm75.c, apbh_dma.c and nand_mxs.c
> >>>> are affected as well of which probably only the first is an issue.)
> >>>
> >>> Sounds good. When we change this we should rename the function
> >>> alongside, because dev_get_drvdata() does something different in Linux.
> >>
> >> Does it? I thought it does the same, with the difference that it can
> >> be set with dev_set_drvdata as well. At the cost of one extra pointer
> >> per device_d, we could have both of them.
> > 
> > dev_set_drvdata() in Linux allows you to store a pointer to private
> > driver data in struct device. We don't have a function for that in
> > barebox and use dev->priv instead.
> > dev_get_drvdata() in barebox gets you the device type data (or however
> > we want to call it). There's no function for that in Linux and we have
> > to first find out if we probe from platform data or from device tree
> > to pick the right function to get the data.
> 
> Ah, I thought the drvdata is pre-populated in Linux. I would rename
> the new function to device_get_match_data then for alignment with
> Linux, with the difference that it returns either platform data or
> device tree driver_data as appropriate.

Linux uses the of_device_get_match_data() [1] for of-based drivers.

[1] https://elixir.bootlin.com/linux/v5.8-rc4/source/drivers/of/device.c#L189

Regards,
  Marco

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH] net:fec: fixed unaligned access and stack corruption

[Ahmad Fatoum]

Hello Marco,

On 7/30/20 11:13 PM, Marco Felsch wrote:
> Hi,
> 
> sorry for jumping in.

No worries,

> On 20-07-11 07:28, Ahmad Fatoum wrote:
>> Ah, I thought the drvdata is pre-populated in Linux. I would rename
>> the new function to device_get_match_data then for alignment with
>> Linux, with the difference that it returns either platform data or
>> device tree driver_data as appropriate.
> 
> Linux uses the of_device_get_match_data() [1] for of-based drivers.
> 
> [1] https://elixir.bootlin.com/linux/v5.8-rc4/source/drivers/of/device.c#L189

Yes, future barebox device_get_match_data should work with both of
and platform data, that's why I'd drop the of_ prefix.

Cheers,
Ahmad

> 
> Regards,
>   Marco
> 

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH] net:fec: fixed unaligned access and stack corruption

[Marco Felsch]

Hi Ahmad,

On 20-07-30 23:23, Ahmad Fatoum wrote:
> Hello Marco,
> 
> On 7/30/20 11:13 PM, Marco Felsch wrote:
> > Hi,
> > 
> > sorry for jumping in.
> 
> No worries,
> 
> > On 20-07-11 07:28, Ahmad Fatoum wrote:
> >> Ah, I thought the drvdata is pre-populated in Linux. I would rename
> >> the new function to device_get_match_data then for alignment with
> >> Linux, with the difference that it returns either platform data or
> >> device tree driver_data as appropriate.
> > 
> > Linux uses the of_device_get_match_data() [1] for of-based drivers.
> > 
> > [1] https://elixir.bootlin.com/linux/v5.8-rc4/source/drivers/of/device.c#L189
> 
> Yes, future barebox device_get_match_data should work with both of
> and platform data, that's why I'd drop the of_ prefix.

I think there is already a device_ wrapper function too but those are
only to differentiate between acpi and of functions. So +1 from my side
:)

Regards,
  Marco

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox