linux-kernel - Re: [PATCH] alarmtimer: Fix rebind failure

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 20 Sep 2023 14:24:23 +0200
From:   Geert Uytterhoeven <geert@...ux-m68k.org>
To:     Biju Das <biju.das.jz@...renesas.com>
Cc:     Thomas Gleixner <tglx@...utronix.de>,
        Alessandro Zummo <a.zummo@...ertech.it>,
        Alexandre Belloni <alexandre.belloni@...tlin.com>,
        John Stultz <jstultz@...gle.com>,
        Stephen Boyd <sboyd@...nel.org>,
        Geert Uytterhoeven <geert+renesas@...der.be>,
        Biju Das <biju.das.au@...il.com>, linux-rtc@...r.kernel.org,
        linux-kernel@...r.kernel.org, linux-renesas-soc@...r.kernel.org
Subject: Re: [PATCH] alarmtimer: Fix rebind failure

Hi Biju,

On Wed, Sep 20, 2023 at 1:59 PM Biju Das <biju.das.jz@...renesas.com> wrote:
> The resources allocated in alarmtimer_rtc_add_device() are not freed
> leading to re-bind failure for the endpoint driver. Fix this issue
> by adding alarmtimer_rtc_remove_device().
>
> Signed-off-by: Biju Das <biju.das.jz@...renesas.com>

Thanks for your patch!

Does this need a Fixes tag?

> --- a/kernel/time/alarmtimer.c
> +++ b/kernel/time/alarmtimer.c
> @@ -61,6 +61,7 @@ static DEFINE_SPINLOCK(freezer_delta_lock);
>  /* rtc timer and device for setting alarm wakeups at suspend */
>  static struct rtc_timer                rtctimer;
>  static struct rtc_device       *rtcdev;
> +static struct platform_device  *rtc_pdev;
>  static DEFINE_SPINLOCK(rtcdev_lock);
>
>  /**
> @@ -109,6 +110,7 @@ static int alarmtimer_rtc_add_device(struct device *dev)
>                 }
>
>                 rtcdev = rtc;
> +               rtc_pdev = pdev;
>                 /* hold a reference so it doesn't go away */
>                 get_device(dev);
>                 pdev = NULL;
> @@ -123,6 +125,23 @@ static int alarmtimer_rtc_add_device(struct device *dev)
>         return ret;
>  }
>
> +static void alarmtimer_rtc_remove_device(struct device *dev)
> +{
> +       struct rtc_device *rtc = to_rtc_device(dev);
> +
> +       if (rtc_pdev) {

As the return value of class_interface.add_dev() is never checked
(alarmtimer_rtc_add_device() returns -EBUSY on adding a second
alarmtimer), multiple timers may have been added, but only one of them
will be the real alarmtimer.
Hence this function should check if rtcdev == rtc before unregistering
the real alarmtimer.  Of course all of this should be protected by
rtcdev_lock.

> +               module_put(rtc->owner);
> +               if (device_may_wakeup(rtc->dev.parent))
> +                       device_init_wakeup(&rtc_pdev->dev, false);
> +
> +               platform_device_unregister(rtc_pdev);
> +               put_device(dev);

Perhaps use the reverse order of operations as in
alarmtimer_rtc_add_device()?

> +       }
> +
> +       rtcdev = NULL;
> +       rtc_pdev = NULL;
> +}
> +
>  static inline void alarmtimer_rtc_timer_init(void)
>  {
>         rtc_timer_init(&rtctimer, NULL, NULL);

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@...ux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds