diff mbox series

[v2,1/2] i2c: dev: fix notifier return values

Message ID 20221229160045.535778-2-brgl@bgdev.pl
State Accepted
Headers show
Series i2c: fortify the subsystem against user-space induced deadlocks | expand

Commit Message

Bartosz Golaszewski Dec. 29, 2022, 4 p.m. UTC 
From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

We have a set of return values that notifier callbacks can return. They
should not return 0, error codes or anything other than those predefined
values. Make the i2c character device's callback return NOTIFY_DONE or
NOTIFY_OK depending on the situation.

Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
 drivers/i2c/i2c-dev.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

Comments

Wolfram Sang Jan. 17, 2023, 10:04 a.m. UTC | #1 
On Thu, Dec 29, 2022 at 05:00:44PM +0100, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> 
> We have a set of return values that notifier callbacks can return. They
> should not return 0, error codes or anything other than those predefined
> values. Make the i2c character device's callback return NOTIFY_DONE or
> NOTIFY_OK depending on the situation.
> 
> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

Applied to for-next, thanks!

I start reviewing patch 2 now...
Geert Uytterhoeven March 8, 2023, 4:58 p.m. UTC | #2 
Hi Bartosz,

On Thu, Dec 29, 2022 at 5:12 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
>
> We have a set of return values that notifier callbacks can return. They
> should not return 0, error codes or anything other than those predefined
> values. Make the i2c character device's callback return NOTIFY_DONE or
> NOTIFY_OK depending on the situation.
>
> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

Thanks for your patch, which is now commit cddf70d0bce71c2a ("i2c:
dev: fix notifier return values") in v6.3-rc1.

On SH/R-Mobile platforms, this leads to missing /dev/i2c-* entries.
On R-Car Gen4, they are still present, as all I2C adapters are
initialized after i2cdev.

> --- a/drivers/i2c/i2c-dev.c
> +++ b/drivers/i2c/i2c-dev.c
> @@ -653,12 +653,12 @@ static int i2cdev_attach_adapter(struct device *dev, void *dummy)
>         int res;
>
>         if (dev->type != &i2c_adapter_type)
> -               return 0;
> +               return NOTIFY_DONE;
>         adap = to_i2c_adapter(dev);
>
>         i2c_dev = get_free_i2c_dev(adap);
>         if (IS_ERR(i2c_dev))
> -               return PTR_ERR(i2c_dev);
> +               return NOTIFY_DONE;
>
>         cdev_init(&i2c_dev->cdev, &i2cdev_fops);
>         i2c_dev->cdev.owner = THIS_MODULE;
> @@ -678,11 +678,11 @@ static int i2cdev_attach_adapter(struct device *dev, void *dummy)
>                 goto err_put_i2c_dev;
>
>         pr_debug("adapter [%s] registered as minor %d\n", adap->name, adap->nr);
> -       return 0;
> +       return NOTIFY_OK;

Unfortunately i2cdev_{at,de}tach_adapter() are not only used as
notifiers (called from i2cdev_notifier_call()), but also called from
i2c_dev_init():

        /* Bind to already existing adapters right away */
        i2c_for_each_dev(NULL, i2cdev_attach_adapter);

and i2c_dev_exit():

        i2c_for_each_dev(NULL, i2cdev_detach_adapter);

As soon i2c_dev_{at,de}tach_adapter() returns a non-zero
value (e.g. NOTIFY_OK), {i2c,bus}_for_each_dev() aborts
processing.

In i2c_dev_init(), this leads to a failure in registering any
already existing i2c adapters after the first one, causing missing
/dev/i2c-* entries.

In i2c_dev_exit(), this leads to a failure unregistering any but the
first i2c adapter.

As there is no one-to-one mapping from error codes to notify codes,
I think this cannot just be handled inside i2cdev_notifier_call() :-(

Gr{oetje,eeting}s,

                        Geert
Bartosz Golaszewski March 8, 2023, 7:33 p.m. UTC | #3 
On Wed, Mar 8, 2023 at 5:58 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>
> Hi Bartosz,
>
> On Thu, Dec 29, 2022 at 5:12 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> >
> > We have a set of return values that notifier callbacks can return. They
> > should not return 0, error codes or anything other than those predefined
> > values. Make the i2c character device's callback return NOTIFY_DONE or
> > NOTIFY_OK depending on the situation.
> >
> > Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
>
> Thanks for your patch, which is now commit cddf70d0bce71c2a ("i2c:
> dev: fix notifier return values") in v6.3-rc1.
>
> On SH/R-Mobile platforms, this leads to missing /dev/i2c-* entries.
> On R-Car Gen4, they are still present, as all I2C adapters are
> initialized after i2cdev.
>
> > --- a/drivers/i2c/i2c-dev.c
> > +++ b/drivers/i2c/i2c-dev.c
> > @@ -653,12 +653,12 @@ static int i2cdev_attach_adapter(struct device *dev, void *dummy)
> >         int res;
> >
> >         if (dev->type != &i2c_adapter_type)
> > -               return 0;
> > +               return NOTIFY_DONE;
> >         adap = to_i2c_adapter(dev);
> >
> >         i2c_dev = get_free_i2c_dev(adap);
> >         if (IS_ERR(i2c_dev))
> > -               return PTR_ERR(i2c_dev);
> > +               return NOTIFY_DONE;
> >
> >         cdev_init(&i2c_dev->cdev, &i2cdev_fops);
> >         i2c_dev->cdev.owner = THIS_MODULE;
> > @@ -678,11 +678,11 @@ static int i2cdev_attach_adapter(struct device *dev, void *dummy)
> >                 goto err_put_i2c_dev;
> >
> >         pr_debug("adapter [%s] registered as minor %d\n", adap->name, adap->nr);
> > -       return 0;
> > +       return NOTIFY_OK;
>
> Unfortunately i2cdev_{at,de}tach_adapter() are not only used as
> notifiers (called from i2cdev_notifier_call()), but also called from
> i2c_dev_init():
>
>         /* Bind to already existing adapters right away */
>         i2c_for_each_dev(NULL, i2cdev_attach_adapter);
>
> and i2c_dev_exit():
>
>         i2c_for_each_dev(NULL, i2cdev_detach_adapter);
>
> As soon i2c_dev_{at,de}tach_adapter() returns a non-zero
> value (e.g. NOTIFY_OK), {i2c,bus}_for_each_dev() aborts
> processing.
>
> In i2c_dev_init(), this leads to a failure in registering any
> already existing i2c adapters after the first one, causing missing
> /dev/i2c-* entries.
>
> In i2c_dev_exit(), this leads to a failure unregistering any but the
> first i2c adapter.
>
> As there is no one-to-one mapping from error codes to notify codes,
> I think this cannot just be handled inside i2cdev_notifier_call() :-(
>

Would wrapping i2c_a/detach_adapter() in a notifier callback work? So
that SH can call it directly while notifiers would call it indirectly
through the wrapper?

Bart
Geert Uytterhoeven March 8, 2023, 7:51 p.m. UTC | #4 
Hi Bartosz,

On Wed, Mar 8, 2023 at 8:33 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> On Wed, Mar 8, 2023 at 5:58 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > On Thu, Dec 29, 2022 at 5:12 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> > > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> > >
> > > We have a set of return values that notifier callbacks can return. They
> > > should not return 0, error codes or anything other than those predefined
> > > values. Make the i2c character device's callback return NOTIFY_DONE or
> > > NOTIFY_OK depending on the situation.
> > >
> > > Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> >
> > Thanks for your patch, which is now commit cddf70d0bce71c2a ("i2c:
> > dev: fix notifier return values") in v6.3-rc1.
> >
> > On SH/R-Mobile platforms, this leads to missing /dev/i2c-* entries.
> > On R-Car Gen4, they are still present, as all I2C adapters are
> > initialized after i2cdev.
> >
> > > --- a/drivers/i2c/i2c-dev.c
> > > +++ b/drivers/i2c/i2c-dev.c
> > > @@ -653,12 +653,12 @@ static int i2cdev_attach_adapter(struct device *dev, void *dummy)
> > >         int res;
> > >
> > >         if (dev->type != &i2c_adapter_type)
> > > -               return 0;
> > > +               return NOTIFY_DONE;
> > >         adap = to_i2c_adapter(dev);
> > >
> > >         i2c_dev = get_free_i2c_dev(adap);
> > >         if (IS_ERR(i2c_dev))
> > > -               return PTR_ERR(i2c_dev);
> > > +               return NOTIFY_DONE;
> > >
> > >         cdev_init(&i2c_dev->cdev, &i2cdev_fops);
> > >         i2c_dev->cdev.owner = THIS_MODULE;
> > > @@ -678,11 +678,11 @@ static int i2cdev_attach_adapter(struct device *dev, void *dummy)
> > >                 goto err_put_i2c_dev;
> > >
> > >         pr_debug("adapter [%s] registered as minor %d\n", adap->name, adap->nr);
> > > -       return 0;
> > > +       return NOTIFY_OK;
> >
> > Unfortunately i2cdev_{at,de}tach_adapter() are not only used as
> > notifiers (called from i2cdev_notifier_call()), but also called from
> > i2c_dev_init():
> >
> >         /* Bind to already existing adapters right away */
> >         i2c_for_each_dev(NULL, i2cdev_attach_adapter);
> >
> > and i2c_dev_exit():
> >
> >         i2c_for_each_dev(NULL, i2cdev_detach_adapter);
> >
> > As soon i2c_dev_{at,de}tach_adapter() returns a non-zero
> > value (e.g. NOTIFY_OK), {i2c,bus}_for_each_dev() aborts
> > processing.
> >
> > In i2c_dev_init(), this leads to a failure in registering any
> > already existing i2c adapters after the first one, causing missing
> > /dev/i2c-* entries.
> >
> > In i2c_dev_exit(), this leads to a failure unregistering any but the
> > first i2c adapter.
> >
> > As there is no one-to-one mapping from error codes to notify codes,
> > I think this cannot just be handled inside i2cdev_notifier_call() :-(
>
> Would wrapping i2c_a/detach_adapter() in a notifier callback work? So
> that SH can call it directly while notifiers would call it indirectly
> through the wrapper?

That would be a wrapper that ignores the NOTIFY_* return
value, and always returns zero? I.e. we can no longer return an
error.  I guess that's OK, as i2c_dev_init() doesn't take any
action based on the returned error code anyway.

The only error conditions that can happen in i2c_attach_adapter()
are:
  - "Out of device minors" message in get_free_i2c_dev(),
  - WARN_ON(dev == WHITEOUT_DEV) in cdev_add(),
  - Generic -ENOMEM.
Looks like all of the above can be ignored, as they are all unlikely to
happen, and there is nothing to be done to recover...

Note that this is not "called directly from SH".
The SH/R-Mobile SoCs where I noticed the issue are ARM32.
I guess it can happen on other platforms, too, depending on initialization
order...

Gr{oetje,eeting}s,

                        Geert
Geert Uytterhoeven March 9, 2023, 7:47 a.m. UTC | #5 
Hi Bartosz,

On Wed, Mar 8, 2023 at 8:51 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> On Wed, Mar 8, 2023 at 8:33 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> > On Wed, Mar 8, 2023 at 5:58 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > > On Thu, Dec 29, 2022 at 5:12 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> > > > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> > > >
> > > > We have a set of return values that notifier callbacks can return. They
> > > > should not return 0, error codes or anything other than those predefined
> > > > values. Make the i2c character device's callback return NOTIFY_DONE or
> > > > NOTIFY_OK depending on the situation.
> > > >
> > > > Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> > >
> > > Thanks for your patch, which is now commit cddf70d0bce71c2a ("i2c:
> > > dev: fix notifier return values") in v6.3-rc1.
> > >
> > > On SH/R-Mobile platforms, this leads to missing /dev/i2c-* entries.
> > > On R-Car Gen4, they are still present, as all I2C adapters are
> > > initialized after i2cdev.
> > >
> > > > --- a/drivers/i2c/i2c-dev.c
> > > > +++ b/drivers/i2c/i2c-dev.c
> > > > @@ -653,12 +653,12 @@ static int i2cdev_attach_adapter(struct device *dev, void *dummy)
> > > >         int res;
> > > >
> > > >         if (dev->type != &i2c_adapter_type)
> > > > -               return 0;
> > > > +               return NOTIFY_DONE;
> > > >         adap = to_i2c_adapter(dev);
> > > >
> > > >         i2c_dev = get_free_i2c_dev(adap);
> > > >         if (IS_ERR(i2c_dev))
> > > > -               return PTR_ERR(i2c_dev);
> > > > +               return NOTIFY_DONE;
> > > >
> > > >         cdev_init(&i2c_dev->cdev, &i2cdev_fops);
> > > >         i2c_dev->cdev.owner = THIS_MODULE;
> > > > @@ -678,11 +678,11 @@ static int i2cdev_attach_adapter(struct device *dev, void *dummy)
> > > >                 goto err_put_i2c_dev;
> > > >
> > > >         pr_debug("adapter [%s] registered as minor %d\n", adap->name, adap->nr);
> > > > -       return 0;
> > > > +       return NOTIFY_OK;
> > >
> > > Unfortunately i2cdev_{at,de}tach_adapter() are not only used as
> > > notifiers (called from i2cdev_notifier_call()), but also called from
> > > i2c_dev_init():
> > >
> > >         /* Bind to already existing adapters right away */
> > >         i2c_for_each_dev(NULL, i2cdev_attach_adapter);
> > >
> > > and i2c_dev_exit():
> > >
> > >         i2c_for_each_dev(NULL, i2cdev_detach_adapter);
> > >
> > > As soon i2c_dev_{at,de}tach_adapter() returns a non-zero
> > > value (e.g. NOTIFY_OK), {i2c,bus}_for_each_dev() aborts
> > > processing.
> > >
> > > In i2c_dev_init(), this leads to a failure in registering any
> > > already existing i2c adapters after the first one, causing missing
> > > /dev/i2c-* entries.
> > >
> > > In i2c_dev_exit(), this leads to a failure unregistering any but the
> > > first i2c adapter.
> > >
> > > As there is no one-to-one mapping from error codes to notify codes,
> > > I think this cannot just be handled inside i2cdev_notifier_call() :-(
> >
> > Would wrapping i2c_a/detach_adapter() in a notifier callback work? So
> > that SH can call it directly while notifiers would call it indirectly
> > through the wrapper?
>
> That would be a wrapper that ignores the NOTIFY_* return
> value, and always returns zero? I.e. we can no longer return an
> error.  I guess that's OK, as i2c_dev_init() doesn't take any
> action based on the returned error code anyway.

This works, so I've sent a fix
https://lore.kernel.org/r/03a8cd13af352c4d990bc70b72df4915b9fa2874.1678347776.git.geert+renesas@glider.be

Gr{oetje,eeting}s,

                        Geert
diff mbox series

Patch

diff --git a/drivers/i2c/i2c-dev.c b/drivers/i2c/i2c-dev.c
index ab0adaa130da..107623c4cc14 100644
--- a/drivers/i2c/i2c-dev.c
+++ b/drivers/i2c/i2c-dev.c
@@ -653,12 +653,12 @@  static int i2cdev_attach_adapter(struct device *dev, void *dummy)
 	int res;
 
 	if (dev->type != &i2c_adapter_type)
-		return 0;
+		return NOTIFY_DONE;
 	adap = to_i2c_adapter(dev);
 
 	i2c_dev = get_free_i2c_dev(adap);
 	if (IS_ERR(i2c_dev))
-		return PTR_ERR(i2c_dev);
+		return NOTIFY_DONE;
 
 	cdev_init(&i2c_dev->cdev, &i2cdev_fops);
 	i2c_dev->cdev.owner = THIS_MODULE;
@@ -678,11 +678,11 @@  static int i2cdev_attach_adapter(struct device *dev, void *dummy)
 		goto err_put_i2c_dev;
 
 	pr_debug("adapter [%s] registered as minor %d\n", adap->name, adap->nr);
-	return 0;
+	return NOTIFY_OK;
 
 err_put_i2c_dev:
 	put_i2c_dev(i2c_dev, false);
-	return res;
+	return NOTIFY_DONE;
 }
 
 static int i2cdev_detach_adapter(struct device *dev, void *dummy)
@@ -691,17 +691,17 @@  static int i2cdev_detach_adapter(struct device *dev, void *dummy)
 	struct i2c_dev *i2c_dev;
 
 	if (dev->type != &i2c_adapter_type)
-		return 0;
+		return NOTIFY_DONE;
 	adap = to_i2c_adapter(dev);
 
 	i2c_dev = i2c_dev_get_by_minor(adap->nr);
 	if (!i2c_dev) /* attach_adapter must have failed */
-		return 0;
+		return NOTIFY_DONE;
 
 	put_i2c_dev(i2c_dev, true);
 
 	pr_debug("adapter [%s] unregistered\n", adap->name);
-	return 0;
+	return NOTIFY_OK;
 }
 
 static int i2cdev_notifier_call(struct notifier_block *nb, unsigned long action,
@@ -716,7 +716,7 @@  static int i2cdev_notifier_call(struct notifier_block *nb, unsigned long action,
 		return i2cdev_detach_adapter(dev, NULL);
 	}
 
-	return 0;
+	return NOTIFY_DONE;
 }
 
 static struct notifier_block i2cdev_notifier = {