linux-kernel - Re: [PATCH] staging: ks7010: select CRYPTO_HASH/CRYPTO_MICHAEL

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, 13 Oct 2021 15:03:41 +0200
From:   Arnd Bergmann <arnd@...db.de>
To:     Vegard Nossum <vegard.nossum@...cle.com>
Cc:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Arnd Bergmann <arnd@...db.de>, linux-staging@...ts.linux.dev,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Randy Dunlap <rdunlap@...radead.org>,
        Herbert Xu <herbert@...dor.apana.org.au>,
        "David S. Miller" <davem@...emloft.net>,
        "open list:HARDWARE RANDOM NUMBER GENERATOR CORE" 
        <linux-crypto@...r.kernel.org>
Subject: Re: [PATCH] staging: ks7010: select CRYPTO_HASH/CRYPTO_MICHAEL_MIC

On Wed, Oct 13, 2021 at 2:51 PM Vegard Nossum <vegard.nossum@...cle.com> wrote:
> On 10/13/21 2:26 PM, Greg Kroah-Hartman wrote:
> > On Mon, Oct 11, 2021 at 05:29:41PM +0200, Vegard Nossum wrote:
> >> Fix the following build/link errors:
> >>
> >>   ld: drivers/staging/ks7010/ks_hostif.o: in function `michael_mic.constprop.0':
> >>   ks_hostif.c:(.text+0x95b): undefined reference to `crypto_alloc_shash'
> >>   ld: ks_hostif.c:(.text+0x97a): undefined reference to `crypto_shash_setkey'
> >>   ld: ks_hostif.c:(.text+0xa13): undefined reference to `crypto_shash_update'
> >>   ld: ks_hostif.c:(.text+0xa28): undefined reference to `crypto_shash_update'
> >>   ld: ks_hostif.c:(.text+0xa48): undefined reference to `crypto_shash_finup'
> >>   ld: ks_hostif.c:(.text+0xa6d): undefined reference to `crypto_destroy_tfm'
> >>
> >> Signed-off-by: Vegard Nossum <vegard.nossum@...cle.com>
> >> ---
> >>  drivers/staging/ks7010/Kconfig | 3 +++
> >>  1 file changed, 3 insertions(+)
> >>
> >> diff --git a/drivers/staging/ks7010/Kconfig b/drivers/staging/ks7010/Kconfig
> >> index 0987fdc2f70db..8ea6c09286798 100644
> >> --- a/drivers/staging/ks7010/Kconfig
> >> +++ b/drivers/staging/ks7010/Kconfig
> >> @@ -5,6 +5,9 @@ config KS7010
> >>      select WIRELESS_EXT
> >>      select WEXT_PRIV
> >>      select FW_LOADER
> >> +    select CRYPTO
> >> +    select CRYPTO_HASH
> >> +    select CRYPTO_MICHAEL_MIC
> >
> > Let's try to rely on 'depend' and not 'select' please.
>
> I used 'select' because it seemed to be the established pattern for
> these options.

Yes, this is the correct way to do it here. In general, using "depends on"
is better than "select", especially when crossing subsystem boundaries,
however mixing the two is what really hurts because of the circular
dependencies you'd get.

The only way we could use 'depends on' here would be to change all
the other drivers to do the same.

> Compare:
>
> $ find -name '*Kconfig*' | xargs git grep 'depends on CRYPTO$' | wc --lines
> 1
>
> $ find -name '*Kconfig*' | xargs git grep 'select CRYPTO$' | wc --lines
> 66
>
> $ find -name '*Kconfig*' | xargs git grep 'depends on CRYPTO' | wc --lines
> 87
>
> $ find -name '*Kconfig*' | xargs git grep 'select CRYPTO' | wc --lines
> 1005
>
> That said, I have found several other cases where CRYPTO_* algorithms
> are getting 'select'-ed without also selecting CRYPTO/CRYPTO_HASH, so I
> definitely see the problem you're trying to address.
>
> I've added some more people on Cc to see if there is a consensus on the
> best way to do this for the CRYPTO* options going forwards. Thoughts,
> anybody?

I don't think there is much point in trying to change the
existing pattern for crypto. We might want to change some of those
that use 'depends on' at the moment for consistency, though most of
those look correct as well.

        Arnd