[U-Boot] [PATCH] dm: avoid dev->req_seq overflow

[U-Boot] [PATCH] dm: avoid dev->req_seq overflow

[Robert Baldyga]

Since dev->req_seq value is initialized from "reg" property of fdt node,
there is posibility, that address value contained in fdt is greater than
INT_MAX, and then value in dev->req_seq is negative which led to probe()
fail.

This patch fix this problem by ensuring that req_seq is positive, unless
it's one of errno codes.

Signed-off-by: Robert Baldyga <r.baldyga@samsung.com>
---
 drivers/core/device.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/core/device.c b/drivers/core/device.c
index 166b073..35ffce0 100644
--- a/drivers/core/device.c
+++ b/drivers/core/device.c
@@ -107,6 +107,8 @@ int device_bind(struct udevice *parent, struct driver *drv, const char *name,
 	 * when the device is probed.
 	 */
 	dev->req_seq = fdtdec_get_int(gd->fdt_blob, of_offset, "reg", -1);
+	if (!IS_ERR_VALUE(dev->req_seq))
+		dev->req_seq &= INT_MAX;
 	dev->seq = -1;
 	if (uc->uc_drv->name && of_offset != -1) {
 		fdtdec_get_alias_seq(gd->fdt_blob, uc->uc_drv->name, of_offset,
-- 
1.9.1

[U-Boot] [PATCH] dm: avoid dev->req_seq overflow

[Simon Glass]

Hi Robert,

On 18 September 2014 09:13, Robert Baldyga <r.baldyga@samsung.com> wrote:

> Since dev->req_seq value is initialized from "reg" property of fdt node,
> there is posibility, that address value contained in fdt is greater than
> INT_MAX, and then value in dev->req_seq is negative which led to probe()
> fail.
>
> This patch fix this problem by ensuring that req_seq is positive, unless
> it's one of errno codes.
>

Wouldn't this be a bug in the device tree file? What does it mean to have a
-ve value?


> Signed-off-by: Robert Baldyga <r.baldyga@samsung.com>
> ---
>  drivers/core/device.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/drivers/core/device.c b/drivers/core/device.c
> index 166b073..35ffce0 100644
> --- a/drivers/core/device.c
> +++ b/drivers/core/device.c
> @@ -107,6 +107,8 @@ int device_bind(struct udevice *parent, struct driver
> *drv, const char *name,
>          * when the device is probed.
>          */
>         dev->req_seq = fdtdec_get_int(gd->fdt_blob, of_offset, "reg", -1);
> +       if (!IS_ERR_VALUE(dev->req_seq))
> +               dev->req_seq &= INT_MAX;
>         dev->seq = -1;
>         if (uc->uc_drv->name && of_offset != -1) {
>                 fdtdec_get_alias_seq(gd->fdt_blob, uc->uc_drv->name,
> of_offset,
> --
> 1.9.1
>
>
Regards,
Simon

[U-Boot] [PATCH] dm: avoid dev->req_seq overflow

[Robert Baldyga]

On 09/18/2014 08:00 PM, Simon Glass wrote:
> Hi Robert,
> 
> On 18 September 2014 09:13, Robert Baldyga <r.baldyga@samsung.com
> <mailto:r.baldyga@samsung.com>> wrote:
> 
>     Since dev->req_seq value is initialized from "reg" property of fdt node,
>     there is posibility, that address value contained in fdt is greater than
>     INT_MAX, and then value in dev->req_seq is negative which led to probe()
>     fail.
> 
>     This patch fix this problem by ensuring that req_seq is positive, unless
>     it's one of errno codes.
> 
> 
> Wouldn't this be a bug in the device tree file? What does it mean to
> have a -ve value?
> 

Device tree seems to be ok. We have:

	pinctrl0: pinctrl at e0200000 {
		compatible = "samsung,s5pc110-pinctrl";
		reg = <0xe0200000 0x1000>;
	};

So when we take address from "reg" as dev->req_seq, then value
0xe0200000 after casting to int gives -534773760. Function
uclass_resolve_seq() returns it as proper seq number, because it's
unique. But then in file drivers/core/device.c, in function
device_probe() we have:

	seq = uclass_resolve_seq(dev);
	if (seq < 0) {
		ret = seq;
		goto fail;
	}

And it will obviously fail.

Using "reg" value as req_seq doesn't work when this value is greater
than INT_MAX.

> 
>     Signed-off-by: Robert Baldyga <r.baldyga@samsung.com
>     <mailto:r.baldyga@samsung.com>>
>     ---
>      drivers/core/device.c | 2 ++
>      1 file changed, 2 insertions(+)
> 
>     diff --git a/drivers/core/device.c b/drivers/core/device.c
>     index 166b073..35ffce0 100644
>     --- a/drivers/core/device.c
>     +++ b/drivers/core/device.c
>     @@ -107,6 +107,8 @@ int device_bind(struct udevice *parent, struct
>     driver *drv, const char *name,
>              * when the device is probed.
>              */
>             dev->req_seq = fdtdec_get_int(gd->fdt_blob, of_offset,
>     "reg", -1);
>     +       if (!IS_ERR_VALUE(dev->req_seq))
>     +               dev->req_seq &= INT_MAX;
>             dev->seq = -1;
>             if (uc->uc_drv->name && of_offset != -1) {
>                     fdtdec_get_alias_seq(gd->fdt_blob, uc->uc_drv->name,
>     of_offset,
>     --
>     1.9.1
> 

Thanks,
Robert Baldyga

[U-Boot] [PATCH] dm: avoid dev->req_seq overflow

[Simon Glass]

Hi Robert,

On 18 September 2014 23:25, Robert Baldyga <r.baldyga@samsung.com> wrote:
>
> On 09/18/2014 08:00 PM, Simon Glass wrote:
> > Hi Robert,
> >
> > On 18 September 2014 09:13, Robert Baldyga <r.baldyga@samsung.com
> > <mailto:r.baldyga@samsung.com>> wrote:
> >
> >     Since dev->req_seq value is initialized from "reg" property of fdt node,
> >     there is posibility, that address value contained in fdt is greater than
> >     INT_MAX, and then value in dev->req_seq is negative which led to probe()
> >     fail.
> >
> >     This patch fix this problem by ensuring that req_seq is positive, unless
> >     it's one of errno codes.
> >
> >
> > Wouldn't this be a bug in the device tree file? What does it mean to
> > have a -ve value?
> >
>
> Device tree seems to be ok. We have:
>
>         pinctrl0: pinctrl at e0200000 {
>                 compatible = "samsung,s5pc110-pinctrl";
>                 reg = <0xe0200000 0x1000>;
>         };
>
> So when we take address from "reg" as dev->req_seq, then value
> 0xe0200000 after casting to int gives -534773760. Function
> uclass_resolve_seq() returns it as proper seq number, because it's
> unique. But then in file drivers/core/device.c, in function
> device_probe() we have:
>
>         seq = uclass_resolve_seq(dev);
>         if (seq < 0) {
>                 ret = seq;
>                 goto fail;
>         }
>
> And it will obviously fail.
>
> Using "reg" value as req_seq doesn't work when this value is greater
> than INT_MAX.

OK I see. Thanks for the clear explanation. We don't really want
req_seq for things that are not buses, but there is no real concept of
that in DM at this stage. Let's see how things pan out. For now, this
patch is a good solution.

Acked-by: Simon Glass <sjg@chromium.org>

Regards,
Simon