linux-kernel - Re: [PATCH] rpmb: use IS_REACHABLE instead of IS

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 for Android: free password hash cracker in your pocket

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

Message-ID: <CAHUa44GgySmc=byL9_0vUuP2BKifpMHqMgvMHJpEukgFGRvVmg@mail.gmail.com>
Date: Mon, 2 Sep 2024 11:54:28 +0200
From: Jens Wiklander <jens.wiklander@...aro.org>
To: Arnd Bergmann <arnd@...db.de>
Cc: linux-kernel@...r.kernel.org, Ulf Hansson <ulf.hansson@...aro.org>, 
	Linus Walleij <linus.walleij@...aro.org>, Greg Kroah-Hartman <gregkh@...uxfoundation.org>, 
	kernel test robot <lkp@...el.com>
Subject: Re: [PATCH] rpmb: use IS_REACHABLE instead of IS_ENABLED

Hi Arnd,

On Mon, Sep 2, 2024 at 10:31 AM Arnd Bergmann <arnd@...db.de> wrote:
>
> On Mon, Sep 2, 2024, at 08:07, Jens Wiklander wrote:
> > Use the macro IS_REACHABLE instead of IS_ENABLED in <linux/rpmb.h> when
> > deciding if prototypes or stubbed static inline functions should be
> > provided. This fixes link errors when the calling code is builtin while
> > the RPMB subsystem is a module.
> >
> > Reported-by: kernel test robot <lkp@...el.com>
> > Closes:
> > https://lore.kernel.org/oe-kbuild-all/202409021448.RSvcBPzt-lkp@intel.com/
> > Fixes: 1e9046e3a154 ("rpmb: add Replay Protected Memory Block (RPMB)
> > subsystem")
> > Signed-off-by: Jens Wiklander <jens.wiklander@...aro.org>
>
> Please don't work around a bug like this, fix it properly instead.
>
> > diff --git a/include/linux/rpmb.h b/include/linux/rpmb.h
> > index cccda73eea4d..37b5273c4027 100644
> > --- a/include/linux/rpmb.h
> > +++ b/include/linux/rpmb.h
> > @@ -61,7 +61,7 @@ struct rpmb_dev {
> >
> >  #define to_rpmb_dev(x)               container_of((x), struct rpmb_dev, dev)
> >
> > -#if IS_ENABLED(CONFIG_RPMB)
> > +#if IS_REACHABLE(CONFIG_RPMB)
> >  struct rpmb_dev *rpmb_dev_get(struct rpmb_dev *rdev);
> >  void rpmb_dev_put(struct rpmb_dev *rdev);
>
> This gives very unexpected runtime behavior where both RPMB and
> its user are enabled, but it doesn't work.
>
> I think what you want here is a dependency like
>
>       depends on RPMB || !RPMB
>
> for every caller. This enforces at build time that the MMC core can
> be built either when RPMB is disabled, of when it is reachable.

Thanks for the suggestion.

I tried adding the dependency above for MMC_BLOCK and OPTEE.

Setting CONFIG_RPMB to "y" works as expected CONFIG_MMC_BLOCK and
CONFIG_OPTEE can be configured as both "y" or "m".

Setting CONFIG_RPMB to "m" will make CONFIG_MMC_BLOCK and CONFIG_OPTEE
"m" too, even if they are set as "y" in the defconfig. So the module
status seems to spread to the options with the dependency. For
tristate options, it guarantees that the expected features are
available even if it changes from built-in to a loadable module.

It will do nothing for bool options, but that's a hypothetical case
for now.  Wouldn't spreading the built-in status to CONFIG_RPMB be
better? So CONFIG_RPMB turns into "y" if an option configured as
built-in depends on it. I don't know how to do that though, so that
could be saved for when it's needed.

I'll send out patches for MMC_BLOCK and OPTEE options shortly if we're
happy with the "depends on RPMB || !RPMB" approach.

Cheers,
Jens