[PATCH 0/2] KEYS: Revert and replace incorrect UAPI fix

[PATCH 0/2] KEYS: Revert and replace incorrect UAPI fix

[David Howells]

Hi James,

Here's a pair of fixes that need to go upstream asap, please:

 (1) Revert an incorrect fix to the keyrings UAPI for a C++ reserved word
     used as a struct member name.  This change being reverted breaks
     existing userspace code and is thus incorrect.

     Further, *neither* name is consistent with the one in the keyutils
     package public header.

 (2) Fix the problem by using a union to make the name from keyutils
     available in parallel and make the 'private' name unavailable in C++
     with cpp-conditionals.

David
---
David Howells (1):
      keys: Fix the use of the C++ keyword "private" in uapi/linux/keyctl.h

Lubomir Rintel (1):
      Revert "uapi/linux/keyctl.h: don't use C++ reserved keyword as a struct member name"


 include/uapi/linux/keyctl.h |    7 ++++++-
 security/keys/dh.c          |    2 +-
 2 files changed, 7 insertions(+), 2 deletions(-)

[PATCH 1/2] Revert "uapi/linux/keyctl.h: don't use C++ reserved keyword as a struct member name"

[David Howells]

From: Lubomir Rintel <lkundrak@v3.sk>

This changes UAPI, breaking iwd and libell:

  ell/key.c: In function 'kernel_dh_compute':
  ell/key.c:205:38: error: 'struct keyctl_dh_params' has no member named 'private'; did you mean 'dh_private'?
    struct keyctl_dh_params params = { .private = private,
                                        ^~~~~~~
                                        dh_private

This reverts commit 8a2336e549d385bb0b46880435b411df8d8200e8.

Signed-off-by: Lubomir Rintel <lkundrak@v3.sk>
Signed-off-by: David Howells <dhowells@redhat.com>
cc: Randy Dunlap <rdunlap@infradead.org>
cc: Mat Martineau <mathew.j.martineau@linux.intel.com>
cc: Stephan Mueller <smueller@chronox.de>
cc: James Morris <jmorris@namei.org>
cc: "Serge E. Hallyn" <serge@hallyn.com>
cc: Mat Martineau <mathew.j.martineau@linux.intel.com>
cc: Andrew Morton <akpm@linux-foundation.org>
cc: Linus Torvalds <torvalds@linux-foundation.org>
cc: <stable@vger.kernel.org>
---

 include/uapi/linux/keyctl.h |    2 +-
 security/keys/dh.c          |    2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/include/uapi/linux/keyctl.h b/include/uapi/linux/keyctl.h
index 910cc4334b21..7b8c9e19bad1 100644
--- a/include/uapi/linux/keyctl.h
+++ b/include/uapi/linux/keyctl.h
@@ -65,7 +65,7 @@
 
 /* keyctl structures */
 struct keyctl_dh_params {
-	__s32 dh_private;
+	__s32 private;
 	__s32 prime;
 	__s32 base;
 };
diff --git a/security/keys/dh.c b/security/keys/dh.c
index 3b602a1e27fa..711e89d8c415 100644
--- a/security/keys/dh.c
+++ b/security/keys/dh.c
@@ -300,7 +300,7 @@ long __keyctl_dh_compute(struct keyctl_dh_params __user *params,
 	}
 	dh_inputs.g_size = dlen;
 
-	dlen = dh_data_from_key(pcopy.dh_private, &dh_inputs.key);
+	dlen = dh_data_from_key(pcopy.private, &dh_inputs.key);
 	if (dlen < 0) {
 		ret = dlen;
 		goto out2;

[PATCH 2/2] keys: Fix the use of the C++ keyword "private" in uapi/linux/keyctl.h

[David Howells]

The keyctl_dh_params struct in uapi/linux/keyctl.h contains the symbol
"private" which means that the header file will cause compilation failure
if #included in to a C++ program.  Further, the patch that added the same
struct to the keyutils package named the symbol "priv", not "private".

The previous attempt to fix this (commit 8a2336e549d3) did so by simply
renaming the kernel's copy of the field to dh_private, but this then breaks
existing userspace and as such has to be reverted (which is done by the
preceding patch).

[And note, to those who think that wrapping the struct in extern "C" {}
 will work: it won't; that only changes how symbol names are presented to
 the assembler and linker.].

Instead, insert an anonymous union around the "private" member and add a
second member in there with the name "priv" to match the one in the
keyutils package.  The "private" member is then wrapped in !__cplusplus
cpp-conditionals to hide it from C++.

Fixes: ddbb41148724 ("KEYS: Add KEYCTL_DH_COMPUTE command")
Fixes: 8a2336e549d3 ("uapi/linux/keyctl.h: don't use C++ reserved keyword as a struct member name")
Signed-off-by: David Howells <dhowells@redhat.com>
cc: Randy Dunlap <rdunlap@infradead.org>
cc: Lubomir Rintel <lkundrak@v3.sk>
cc: James Morris <jmorris@namei.org>
cc: Mat Martineau <mathew.j.martineau@linux.intel.com>
cc: Stephan Mueller <smueller@chronox.de>
cc: Andrew Morton <akpm@linux-foundation.org>
cc: Linus Torvalds <torvalds@linux-foundation.org>
cc: stable@vger.kernel.org
---

 include/uapi/linux/keyctl.h |    7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/include/uapi/linux/keyctl.h b/include/uapi/linux/keyctl.h
index 7b8c9e19bad1..0f3cb13db8e9 100644
--- a/include/uapi/linux/keyctl.h
+++ b/include/uapi/linux/keyctl.h
@@ -65,7 +65,12 @@
 
 /* keyctl structures */
 struct keyctl_dh_params {
-	__s32 private;
+	union {
+#ifndef __cplusplus
+		__s32 private;
+#endif
+		__s32 priv;
+	};
 	__s32 prime;
 	__s32 base;
 };

Re: [PATCH 2/2] keys: Fix the use of the C++ keyword "private" in uapi/linux/keyctl.h

[Lubomir Rintel]

On Mon, 2018-09-24 at 13:18 +0100, David Howells wrote:
> The keyctl_dh_params struct in uapi/linux/keyctl.h contains the
> symbol
> "private" which means that the header file will cause compilation
> failure
> if #included in to a C++ program.  Further, the patch that added the
> same
> struct to the keyutils package named the symbol "priv", not
> "private".
> 
> The previous attempt to fix this (commit 8a2336e549d3) did so by
> simply
> renaming the kernel's copy of the field to dh_private, but this then
> breaks
> existing userspace and as such has to be reverted (which is done by
> the
> preceding patch).
> 
> [And note, to those who think that wrapping the struct in extern "C"
> {}
>  will work: it won't; that only changes how symbol names are
> presented to
>  the assembler and linker.].
> 
> Instead, insert an anonymous union around the "private" member and
> add a
> second member in there with the name "priv" to match the one in the
> keyutils package.  The "private" member is then wrapped in
> !__cplusplus
> cpp-conditionals to hide it from C++.
> 
> Fixes: ddbb41148724 ("KEYS: Add KEYCTL_DH_COMPUTE command")
> Fixes: 8a2336e549d3 ("uapi/linux/keyctl.h: don't use C++ reserved
> keyword as a struct member name")
> Signed-off-by: David Howells <dhowells@redhat.com>
> cc: Randy Dunlap <rdunlap@infradead.org>
> cc: Lubomir Rintel <lkundrak@v3.sk>
> cc: James Morris <jmorris@namei.org>
> cc: Mat Martineau <mathew.j.martineau@linux.intel.com>
> cc: Stephan Mueller <smueller@chronox.de>
> cc: Andrew Morton <akpm@linux-foundation.org>
> cc: Linus Torvalds <torvalds@linux-foundation.org>
> cc: stable@vger.kernel.org
> ---
> 
>  include/uapi/linux/keyctl.h |    7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/include/uapi/linux/keyctl.h
> b/include/uapi/linux/keyctl.h
> index 7b8c9e19bad1..0f3cb13db8e9 100644
> --- a/include/uapi/linux/keyctl.h
> +++ b/include/uapi/linux/keyctl.h
> @@ -65,7 +65,12 @@
>  
>  /* keyctl structures */
>  struct keyctl_dh_params {
> -	__s32 private;
> +	union {
> +#ifndef __cplusplus
> +		__s32 private;
> +#endif
> +		__s32 priv;
> +	};
>  	__s32 prime;
>  	__s32 base;
>  };
> 

Thank you, checked that this works fine for libell build.

Tested-by: Lubomir Rintel <lkundrak@v3.sk>

Perhaps you may want to call the C++ friendly field "dh_private",
because there might be C++ users that utilize it already. (The chance
might be negligible though.).

Lubo

Re: [PATCH 2/2] keys: Fix the use of the C++ keyword "private" in uapi/linux/keyctl.h

[Mat Martineau]

On Mon, 24 Sep 2018, David Howells wrote:

> The keyctl_dh_params struct in uapi/linux/keyctl.h contains the symbol
> "private" which means that the header file will cause compilation failure
> if #included in to a C++ program.  Further, the patch that added the same
> struct to the keyutils package named the symbol "priv", not "private".
>
> The previous attempt to fix this (commit 8a2336e549d3) did so by simply
> renaming the kernel's copy of the field to dh_private, but this then breaks
> existing userspace and as such has to be reverted (which is done by the
> preceding patch).
>
> [And note, to those who think that wrapping the struct in extern "C" {}
> will work: it won't; that only changes how symbol names are presented to
> the assembler and linker.].
>
> Instead, insert an anonymous union around the "private" member and add a
> second member in there with the name "priv" to match the one in the
> keyutils package.  The "private" member is then wrapped in !__cplusplus
> cpp-conditionals to hide it from C++.
>
> Fixes: ddbb41148724 ("KEYS: Add KEYCTL_DH_COMPUTE command")
> Fixes: 8a2336e549d3 ("uapi/linux/keyctl.h: don't use C++ reserved keyword as a struct member name")
> Signed-off-by: David Howells <dhowells@redhat.com>
> cc: Randy Dunlap <rdunlap@infradead.org>
> cc: Lubomir Rintel <lkundrak@v3.sk>
> cc: James Morris <jmorris@namei.org>
> cc: Mat Martineau <mathew.j.martineau@linux.intel.com>
> cc: Stephan Mueller <smueller@chronox.de>
> cc: Andrew Morton <akpm@linux-foundation.org>
> cc: Linus Torvalds <torvalds@linux-foundation.org>
> cc: stable@vger.kernel.org
> ---
>
> include/uapi/linux/keyctl.h |    7 ++++++-
> 1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/include/uapi/linux/keyctl.h b/include/uapi/linux/keyctl.h
> index 7b8c9e19bad1..0f3cb13db8e9 100644
> --- a/include/uapi/linux/keyctl.h
> +++ b/include/uapi/linux/keyctl.h
> @@ -65,7 +65,12 @@
>
> /* keyctl structures */
> struct keyctl_dh_params {
> -	__s32 private;
> +	union {
> +#ifndef __cplusplus
> +		__s32 private;
> +#endif
> +		__s32 priv;
> +	};
> 	__s32 prime;
> 	__s32 base;
> };
>
>

Thanks David. This is an important fix for existing C userspace code, it 
would create a headache now and in to the future if the keyctl.h change 
introduced in v4.19-rc3 made it in to an official release. Please merge, 
James!

Reviewed-By: Mat Martineau <mathew.j.martineau@linux.intel.com>

--
Mat Martineau
Intel OTC