| 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
| ||
|
Message-ID: <60b7f4c3-c5e8-4fab-bf31-d576fff7a016@suse.de>
Date: Wed, 25 Oct 2023 09:02:02 +0200
From: Hannes Reinecke <hare@...e.de>
To: Arnd Bergmann <arnd@...nel.org>, Keith Busch <kbusch@...nel.org>,
Jens Axboe <axboe@...nel.dk>, Christoph Hellwig <hch@....de>,
Sagi Grimberg <sagi@...mberg.me>,
Chaitanya Kulkarni <kch@...dia.com>
Cc: Arnd Bergmann <arnd@...db.de>, linux-nvme@...ts.infradead.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH] [v2] nvme: keyring: fix conditional compilation
On 10/20/23 22:54, Arnd Bergmann wrote:
> From: Arnd Bergmann <arnd@...db.de>
>
> The keyring and auth functions can be called from both the host and
> the target side and are controlled by Kconfig options for each of the
> combinations, but the declarations are controlled by #ifdef checks
> on the shared Kconfig symbols.
>
> This leads to link failures in combinations where one of the frontends
> is built-in and the other one is a module, and the keyring code
> ends up in a module that is not reachable from the builtin code:
>
> ld: drivers/nvme/host/core.o: in function `nvme_core_exit':
> core.c:(.exit.text+0x4): undefined reference to `nvme_keyring_exit'
> ld: drivers/nvme/host/core.o: in function `nvme_core_init':
> core.c:(.init.text+0x94): undefined reference to `nvme_keyring_init
>
> ld: drivers/nvme/host/tcp.o: in function `nvme_tcp_setup_ctrl':
> tcp.c:(.text+0x4c18): undefined reference to `nvme_tls_psk_default'
>
> aarch64-linux-ld: drivers/nvme/target/configfs.o: in function `nvmet_ports_make':
> configfs.c:(.text+0x3c0c): undefined reference to `nvme_keyring_id'
>
> Address this by wrapping the keyring code in stub functions that are
> used exclusively by one or the other side. In the more complicated auth
> interface, this is done in the separate drivers/nvme/{host,target}/auth.c
> that are conditionally compiled, as well as through large #ifdef blocks,
> but for the simpler keyring interface, it is sufficient to just wrap these
> four functions to ensure that they are only called when the feature is
> enabled in its caller.
>
> In Kconfig, this requires changing the 'select NVME_KEYRING' since the
> keyring calls are done from the host and target core module, which may
> be built-in even when the tcp front-end is in a loadable module.
>
> Fixes: be8e82caa6859 ("nvme-tcp: enable TLS handshake upcall")
> Signed-off-by: Arnd Bergmann <arnd@...db.de>
> ---
> Instead of keying the calls off the Kconfig symbols, replace
> the broken stub helpers with separate ones for host and target
> ---
> drivers/nvme/host/Kconfig | 2 +-
> drivers/nvme/host/core.c | 6 ++--
> drivers/nvme/host/tcp.c | 4 +--
> drivers/nvme/target/Kconfig | 2 +-
> drivers/nvme/target/configfs.c | 4 +--
> include/linux/nvme-keyring.h | 56 +++++++++++++++++++++++++++++-----
> 6 files changed, 57 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/nvme/host/Kconfig b/drivers/nvme/host/Kconfig
> index 8fe2dd619e80e..2d53c23f0a483 100644
> --- a/drivers/nvme/host/Kconfig
> +++ b/drivers/nvme/host/Kconfig
> @@ -2,6 +2,7 @@
> config NVME_CORE
> tristate
> select BLK_DEV_INTEGRITY_T10 if BLK_DEV_INTEGRITY
> + select NVME_KEYRING if NVME_TCP_TLS
>
> config BLK_DEV_NVME
> tristate "NVM Express block device"
> @@ -95,7 +96,6 @@ config NVME_TCP
> config NVME_TCP_TLS
> bool "NVMe over Fabrics TCP TLS encryption support"
> depends on NVME_TCP
> - select NVME_KEYRING
> select NET_HANDSHAKE
> select KEYS
> help
> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> index 62612f87aafa2..c06fea3f8940a 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
> @@ -4724,7 +4724,7 @@ static int __init nvme_core_init(void)
> result = PTR_ERR(nvme_ns_chr_class);
> goto unregister_generic_ns;
> }
> - result = nvme_keyring_init();
> + result = nvme_host_keyring_init();
> if (result)
> goto destroy_ns_chr;
> result = nvme_init_auth();
> @@ -4733,7 +4733,7 @@ static int __init nvme_core_init(void)
> return 0;
>
> keyring_exit:
> - nvme_keyring_exit();
> + nvme_host_keyring_exit();
> destroy_ns_chr:
> class_destroy(nvme_ns_chr_class);
> unregister_generic_ns:
> @@ -4757,7 +4757,7 @@ static int __init nvme_core_init(void)
> static void __exit nvme_core_exit(void)
> {
> nvme_exit_auth();
> - nvme_keyring_exit();
> + nvme_host_keyring_exit();
> class_destroy(nvme_ns_chr_class);
> class_destroy(nvme_subsys_class);
> class_destroy(nvme_class);
> diff --git a/drivers/nvme/host/tcp.c b/drivers/nvme/host/tcp.c
> index 4714a902f4caa..7e4a878b95383 100644
> --- a/drivers/nvme/host/tcp.c
> +++ b/drivers/nvme/host/tcp.c
> @@ -1585,7 +1585,7 @@ static int nvme_tcp_start_tls(struct nvme_ctrl *nctrl,
> int ret;
> struct tls_handshake_args args;
> unsigned long tmo = tls_handshake_timeout * HZ;
> - key_serial_t keyring = nvme_keyring_id();
> + key_serial_t keyring = nvme_host_keyring_id();
>
> dev_dbg(nctrl->device, "queue %d: start TLS with key %x\n",
> qid, pskid);
> @@ -1919,7 +1919,7 @@ static int nvme_tcp_alloc_admin_queue(struct nvme_ctrl *ctrl)
> if (ctrl->opts->tls_key)
> pskid = key_serial(ctrl->opts->tls_key);
> else
> - pskid = nvme_tls_psk_default(ctrl->opts->keyring,
> + pskid = nvme_host_tls_psk_default(ctrl->opts->keyring,
> ctrl->opts->host->nqn,
> ctrl->opts->subsysnqn);
> if (!pskid) {
> diff --git a/drivers/nvme/target/Kconfig b/drivers/nvme/target/Kconfig
> index 31633da9427c7..e5cdf92c628d0 100644
> --- a/drivers/nvme/target/Kconfig
> +++ b/drivers/nvme/target/Kconfig
> @@ -6,6 +6,7 @@ config NVME_TARGET
> depends on CONFIGFS_FS
> select BLK_DEV_INTEGRITY_T10 if BLK_DEV_INTEGRITY
> select SGL_ALLOC
> + select NVME_KEYRING if NVME_TARGET_TCP_TLS
> help
> This enabled target side support for the NVMe protocol, that is
> it allows the Linux kernel to implement NVMe subsystems and
> @@ -87,7 +88,6 @@ config NVME_TARGET_TCP
> config NVME_TARGET_TCP_TLS
> bool "NVMe over Fabrics TCP target TLS encryption support"
> depends on NVME_TARGET_TCP
> - select NVME_KEYRING
> select NET_HANDSHAKE
> select KEYS
> help
> diff --git a/drivers/nvme/target/configfs.c b/drivers/nvme/target/configfs.c
> index 9eed6e6765eaa..337de8da1c178 100644
> --- a/drivers/nvme/target/configfs.c
> +++ b/drivers/nvme/target/configfs.c
> @@ -1893,8 +1893,8 @@ static struct config_group *nvmet_ports_make(struct config_group *group,
> return ERR_PTR(-ENOMEM);
> }
>
> - if (nvme_keyring_id()) {
> - port->keyring = key_lookup(nvme_keyring_id());
> + if (nvme_target_keyring_id()) {
> + port->keyring = key_lookup(nvme_target_keyring_id());
> if (IS_ERR(port->keyring)) {
> pr_warn("NVMe keyring not available, disabling TLS\n");
> port->keyring = NULL;
> diff --git a/include/linux/nvme-keyring.h b/include/linux/nvme-keyring.h
> index 6cc0696625f36..6da4cda7f2f45 100644
> --- a/include/linux/nvme-keyring.h
> +++ b/include/linux/nvme-keyring.h
> @@ -6,8 +6,7 @@
> #ifndef _NVME_KEYRING_H
> #define _NVME_KEYRING_H
>
> -#if IS_ENABLED(CONFIG_NVME_KEYRING)
> -
> +/* internal helpers only, don't call directly */
> key_serial_t nvme_tls_psk_default(struct key *keyring,
> const char *hostnqn, const char *subnqn);
>
> @@ -15,22 +14,63 @@ key_serial_t nvme_keyring_id(void);
> int nvme_keyring_init(void);
> void nvme_keyring_exit(void);
>
> -#else
> +static inline key_serial_t nvme_host_tls_psk_default(struct key *keyring,
> + const char *hostnqn, const char *subnqn)
> +{
> + if (IS_ENABLED(CONFIG_NVME_TCP_TLS))
> + return nvme_tls_psk_default(keyring, hostnqn, subnqn);
> +
> + return 0;
> +}
> +static inline key_serial_t nvme_host_keyring_id(void)
> +{
> + if (IS_ENABLED(CONFIG_NVME_TCP_TLS))
> + return nvme_keyring_id();
> +
> + return 0;
> +}
> +static inline int nvme_host_keyring_init(void)
> +{
> + if (IS_ENABLED(CONFIG_NVME_TCP_TLS))
> + return nvme_keyring_init();
> +
> + return 0;
> +}
> +static inline void nvme_host_keyring_exit(void)
> +{
> + if (IS_ENABLED(CONFIG_NVME_TCP_TLS))
> + nvme_keyring_exit();
> +}
>
> -static inline key_serial_t nvme_tls_psk_default(struct key *keyring,
> +static inline key_serial_t nvme_target_tls_psk_default(struct key *keyring,
> const char *hostnqn, const char *subnqn)
> {
> + if (IS_ENABLED(CONFIG_NVME_TARGET_TCP_TLS))
> + return nvme_host_tls_psk_default(keyring, hostnqn, subnqn);
> +
> return 0;
> }
> -static inline key_serial_t nvme_keyring_id(void)
> +
> +static inline key_serial_t nvme_target_keyring_id(void)
> {
> + if (IS_ENABLED(CONFIG_NVME_TARGET_TCP_TLS))
> + return nvme_keyring_id();
> +
> return 0;
> }
> -static inline int nvme_keyring_init(void)
> +
> +static inline int nvme_target_keyring_init(void)
> {
> + if (IS_ENABLED(CONFIG_NVME_TCP_TLS))
> + return nvme_keyring_init();
> +
> return 0;
> }
> -static inline void nvme_keyring_exit(void) {}
>
> -#endif /* !CONFIG_NVME_KEYRING */
> +static inline void nvme_target_keyring_exit(void)
> +{
> + if (IS_ENABLED(CONFIG_NVME_TARGET_TCP_TLS))
> + nvme_keyring_exit();
> +}
> +
> #endif /* _NVME_KEYRING_H */
I guess the right way is to make 'keyring' a 'real' module, and move
'nvme_keyring_init()' and 'nvme_keyring_exit()' as the modules init/exit
functions. I'll prepare a patch.
Cheers,
Hannes
--
Dr. Hannes Reinecke Kernel Storage Architect
hare@...e.de +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), Geschäftsführer: Ivo Totev, Andrew
Myers, Andrew McDonald, Martje Boudien Moerman
Powered by blists - more mailing lists