linux-kernel - Re: [PATCH] rtc: msc313: Fix function prototype mismatch in msc313_rtc

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] [day] [month] [year] [list]

Date:   Fri, 2 Dec 2022 18:54:07 +0100
From:   Christophe JAILLET <christophe.jaillet@...adoo.fr>
To:     Kees Cook <keescook@...omium.org>
Cc:     Daniel Palmer <daniel@...ngy.jp>,
        kernel test robot <lkp@...el.com>,
        Romain Perier <romain.perier@...il.com>,
        Alessandro Zummo <a.zummo@...ertech.it>,
        Alexandre Belloni <alexandre.belloni@...tlin.com>,
        "Gustavo A. R. Silva" <gustavoars@...nel.org>,
        linux-arm-kernel@...ts.infradead.org, linux-rtc@...r.kernel.org,
        Nathan Chancellor <nathan@...nel.org>,
        Nick Desaulniers <ndesaulniers@...gle.com>,
        Tom Rix <trix@...hat.com>, linux-kernel@...r.kernel.org,
        llvm@...ts.linux.dev, linux-hardening@...r.kernel.org
Subject: Re: [PATCH] rtc: msc313: Fix function prototype mismatch in
 msc313_rtc_probe()

Le 02/12/2022 à 05:46, Kees Cook a écrit :
> On Sat, Nov 19, 2022 at 09:05:28AM +0100, Christophe JAILLET wrote:
>> Le 19/11/2022 à 00:31, Kees Cook a écrit :
>>> With clang's kernel control flow integrity (kCFI, CONFIG_CFI_CLANG),
>>> indirect call targets are validated against the expected function
>>> pointer prototype to make sure the call target is valid to help mitigate
>>> ROP attacks. If they are not identical, there is a failure at run time,
>>> which manifests as either a kernel panic or thread getting killed.
>>>
>>> msc313_rtc_probe() was passing clk_disable_unprepare() directly, which
>>> did not have matching prototypes for devm_add_action_or_reset()'s callback
>>> argument. Add a wrapper and remove the cast.
>>>
>>> This was found as a result of Clang's new -Wcast-function-type-strict
>>> flag, which is more sensitive than the simpler -Wcast-function-type,
>>> which only checks for type width mismatches.
>>>
>>> Reported-by: kernel test robot <lkp@...el.com>
>>> Link: https://lore.kernel.org/lkml/202211041527.HD8TLSE1-lkp@intel.com
>>> Cc: Daniel Palmer <daniel@...ngy.jp>
>>> Cc: Romain Perier <romain.perier@...il.com>
>>> Cc: Alessandro Zummo <a.zummo@...ertech.it>
>>> Cc: Alexandre Belloni <alexandre.belloni@...tlin.com>
>>> Cc: "Gustavo A. R. Silva" <gustavoars@...nel.org>
>>> Cc: linux-arm-kernel@...ts.infradead.org
>>> Cc: linux-rtc@...r.kernel.org
>>> Signed-off-by: Kees Cook <keescook@...omium.org>
>>> ---
>>>    drivers/rtc/rtc-msc313.c | 9 ++++++++-
>>>    1 file changed, 8 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/rtc/rtc-msc313.c b/drivers/rtc/rtc-msc313.c
>>> index f3fde013c4b8..36e3e77f303e 100644
>>> --- a/drivers/rtc/rtc-msc313.c
>>> +++ b/drivers/rtc/rtc-msc313.c
>>> @@ -177,6 +177,13 @@ static irqreturn_t msc313_rtc_interrupt(s32 irq, void *dev_id)
>>>    	return IRQ_HANDLED;
>>>    }
>>> +static void msc313_clk_disable_unprepare(void *data)
>>> +{
>>> +	struct clk *clk = data;
>>> +
>>> +	clk_disable_unprepare(clk);
>>> +}
>>> +
>>>    static int msc313_rtc_probe(struct platform_device *pdev)
>>>    {
>>>    	struct device *dev = &pdev->dev;
>>> @@ -224,7 +231,7 @@ static int msc313_rtc_probe(struct platform_device *pdev)
>>>    		return ret;
>>>    	}
>>> -	ret = devm_add_action_or_reset(dev, (void (*) (void *))clk_disable_unprepare, clk);
>>> +	ret = devm_add_action_or_reset(dev, msc313_clk_disable_unprepare, clk);
>>>    	if (ret)
>>>    		return ret;
>>
>> Hi,
>>
>> another way to fix it, is to use devm_clk_get_enabled().
>>
>> It removes some LoC instead of introducing some new ones and saves a few
>> bytes of memory.
> 
> Hrm, I'm not familiar with the clk stuff here -- how do I use it? Should
> it just be like this? (The NULL argument is ok?)

Yes, the code below looks fine.
NULL argument is ok.

See [1]. Both function are very similar. devm_clk_get_enabled() just 
pass some additional init() and exit() functions.

CJ


[1]: 
https://elixir.bootlin.com/linux/v6.1-rc7/source/drivers/clk/clk-devres.c#L63
> 
> diff --git a/drivers/rtc/rtc-msc313.c b/drivers/rtc/rtc-msc313.c
> index f3fde013c4b8..8d7737e0e2e0 100644
> --- a/drivers/rtc/rtc-msc313.c
> +++ b/drivers/rtc/rtc-msc313.c
> @@ -212,22 +212,12 @@ static int msc313_rtc_probe(struct platform_device *pdev)
>   		return ret;
>   	}
>   
> -	clk = devm_clk_get(dev, NULL);
> +	clk = devm_clk_get_enabled(dev, NULL);
>   	if (IS_ERR(clk)) {
>   		dev_err(dev, "No input reference clock\n");
>   		return PTR_ERR(clk);
>   	}
>   
> -	ret = clk_prepare_enable(clk);
> -	if (ret) {
> -		dev_err(dev, "Failed to enable the reference clock, %d\n", ret);
> -		return ret;
> -	}
> -
> -	ret = devm_add_action_or_reset(dev, (void (*) (void *))clk_disable_unprepare, clk);
> -	if (ret)
> -		return ret;
> -
>   	rate = clk_get_rate(clk);
>   	writew(rate & 0xFFFF, priv->rtc_base + REG_RTC_FREQ_CW_L);
>   	writew((rate >> 16) & 0xFFFF, priv->rtc_base + REG_RTC_FREQ_CW_H);
>