[PATCH 1/5] modpost: fix tee MODULE_DEVICE_TABLE built on big endian host

[PATCH 1/5] modpost: fix tee MODULE_DEVICE_TABLE built on big endian host

[Masahiro Yamada]

When MODULE_DEVICE_TABLE(tee, ) is built on a host with a different
endianness from the target architecture, it results in an incorrect
MODULE_ALIAS().

For example, see a case where drivers/char/hw_random/optee-rng.c
is built as a module.

If you build it on a little endian host, you will get the correct
MODULE_ALIAS:

    $ grep MODULE_ALIAS drivers/char/hw_random/optee-rng.mod.c
    MODULE_ALIAS("tee:ab7a617c-b8e7-4d8f-8301-d09b61036b64*");

However, if you build it on a big endian host, you will get a wrong
MODULE_ALIAS:

    $ grep MODULE_ALIAS drivers/char/hw_random/optee-rng.mod.c
    MODULE_ALIAS("tee:646b0361-9bd0-0183-8f4d-e7b87c617aab*");

This issue has been unnoticed because the ARM kernel is most likely built
on a little endian host (cross-build on x86 or native-build on ARM).

The uuid field must not be reversed because uuid_t is an array of __u8.

Fixes: 0fc1db9d1059 ("tee: add bus driver framework for TEE based devices")
Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
---

 scripts/mod/file2alias.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/scripts/mod/file2alias.c b/scripts/mod/file2alias.c
index 7056751c29b1..70bf6a2f585c 100644
--- a/scripts/mod/file2alias.c
+++ b/scripts/mod/file2alias.c
@@ -1348,13 +1348,13 @@ static int do_typec_entry(const char *filename, void *symval, char *alias)
 /* Looks like: tee:uuid */
 static int do_tee_entry(const char *filename, void *symval, char *alias)
 {
-	DEF_FIELD(symval, tee_client_device_id, uuid);
+	DEF_FIELD_ADDR(symval, tee_client_device_id, uuid);
 
 	sprintf(alias, "tee:%02x%02x%02x%02x-%02x%02x-%02x%02x-%02x%02x-%02x%02x%02x%02x%02x%02x",
-		uuid.b[0], uuid.b[1], uuid.b[2], uuid.b[3], uuid.b[4],
-		uuid.b[5], uuid.b[6], uuid.b[7], uuid.b[8], uuid.b[9],
-		uuid.b[10], uuid.b[11], uuid.b[12], uuid.b[13], uuid.b[14],
-		uuid.b[15]);
+		uuid->b[0], uuid->b[1], uuid->b[2], uuid->b[3], uuid->b[4],
+		uuid->b[5], uuid->b[6], uuid->b[7], uuid->b[8], uuid->b[9],
+		uuid->b[10], uuid->b[11], uuid->b[12], uuid->b[13], uuid->b[14],
+		uuid->b[15]);
 
 	add_wildcard(alias);
 	return 1;
-- 
2.39.2

[PATCH 2/5] modpost: fix ishtp MODULE_DEVICE_TABLE built on big endian host

[Masahiro Yamada]

When MODULE_DEVICE_TABLE(ishtp, ) is built on a host with a different
endianness from the target architecture, it results in an incorrect
MODULE_ALIAS().

For example, see a case where drivers/platform/x86/intel/ishtp_eclite.c
is built as a module.

If you build it on a little endian host, you will get the correct
MODULE_ALIAS:

    $ grep MODULE_ALIAS drivers/platform/x86/intel/ishtp_eclite.mod.c
    MODULE_ALIAS("ishtp:{6A19CC4B-D760-4DE3-B14D-F25EBD0FBCD9}");

However, if you build it on a big endian host, you will get a wrong
MODULE_ALIAS:

    $ grep MODULE_ALIAS drivers/platform/x86/intel/ishtp_eclite.mod.c
    MODULE_ALIAS("ishtp:{BD0FBCD9-F25E-B14D-4DE3-D7606A19CC4B}");

This issue has been unnoticed because the x86 kernel is most likely built
natively on an x86 host.

The guid field must not be reversed because guid_t is an array of __u8.

Fixes: fa443bc3c1e4 ("HID: intel-ish-hid: add support for MODULE_DEVICE_TABLE()")
Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
---

 scripts/mod/file2alias.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/scripts/mod/file2alias.c b/scripts/mod/file2alias.c
index 70bf6a2f585c..6583b36dbe69 100644
--- a/scripts/mod/file2alias.c
+++ b/scripts/mod/file2alias.c
@@ -1401,10 +1401,10 @@ static int do_mhi_ep_entry(const char *filename, void *symval, char *alias)
 /* Looks like: ishtp:{guid} */
 static int do_ishtp_entry(const char *filename, void *symval, char *alias)
 {
-	DEF_FIELD(symval, ishtp_device_id, guid);
+	DEF_FIELD_ADDR(symval, ishtp_device_id, guid);
 
 	strcpy(alias, ISHTP_MODULE_PREFIX "{");
-	add_guid(alias, guid);
+	add_guid(alias, *guid);
 	strcat(alias, "}");
 
 	return 1;
-- 
2.39.2

[PATCH 3/5] modpost: define TO_NATIVE() using bswap_* functions

[Masahiro Yamada]

The current TO_NATIVE() has some limitations:

 1) You cannot cast the argument.

 2) You cannot pass a variable marked as 'const'.

 3) Passing an array is a bug, but it is not detected.

Impelement TO_NATIVE() using bswap_*() functions. These are GNU
extensions. If we face portability issues, we can port the code from
include/uapi/linux/swab.h.

With this change, get_rel_type_and_sym() can be simplified by casting
the arguments directly.

Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
---

 scripts/mod/modpost.c | 13 ++++---------
 scripts/mod/modpost.h | 25 ++++++++++++-------------
 2 files changed, 16 insertions(+), 22 deletions(-)

diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
index 2f3b0fe6f68d..99476a9695c5 100644
--- a/scripts/mod/modpost.c
+++ b/scripts/mod/modpost.c
@@ -1410,15 +1410,10 @@ static void get_rel_type_and_sym(struct elf_info *elf, uint64_t r_info,
 		return;
 	}
 
-	if (is_64bit) {
-		Elf64_Xword r_info64 = r_info;
-
-		r_info = TO_NATIVE(r_info64);
-	} else {
-		Elf32_Word r_info32 = r_info;
-
-		r_info = TO_NATIVE(r_info32);
-	}
+	if (is_64bit)
+		r_info = TO_NATIVE((Elf64_Xword)r_info);
+	else
+		r_info = TO_NATIVE((Elf32_Word)r_info);
 
 	*r_type = ELF_R_TYPE(r_info);
 	*r_sym = ELF_R_SYM(r_info);
diff --git a/scripts/mod/modpost.h b/scripts/mod/modpost.h
index 6413f26fcb6b..1392afec118c 100644
--- a/scripts/mod/modpost.h
+++ b/scripts/mod/modpost.h
@@ -1,4 +1,5 @@
 /* SPDX-License-Identifier: GPL-2.0 */
+#include <byteswap.h>
 #include <stdbool.h>
 #include <stdio.h>
 #include <stdlib.h>
@@ -51,21 +52,19 @@
 #define ELF_R_TYPE  ELF64_R_TYPE
 #endif
 
+#define bswap(x) \
+({ \
+	_Static_assert(sizeof(x) == 1 || sizeof(x) == 2 || \
+		       sizeof(x) == 4 || sizeof(x) == 8, "bug"); \
+	(typeof(x))(sizeof(x) == 2 ? bswap_16(x) : \
+		    sizeof(x) == 4 ? bswap_32(x) : \
+		    sizeof(x) == 8 ? bswap_64(x) : \
+		    x); \
+})
+
 #if KERNEL_ELFDATA != HOST_ELFDATA
 
-static inline void __endian(const void *src, void *dest, unsigned int size)
-{
-	unsigned int i;
-	for (i = 0; i < size; i++)
-		((unsigned char*)dest)[i] = ((unsigned char*)src)[size - i-1];
-}
-
-#define TO_NATIVE(x)						\
-({								\
-	typeof(x) __x;						\
-	__endian(&(x), &(__x), sizeof(__x));			\
-	__x;							\
-})
+#define TO_NATIVE(x) (bswap(x))
 
 #else /* endianness matches */
 
-- 
2.39.2

[PATCH 4/5] modpost: refactor check_sec_ref()

[Masahiro Yamada]

We can replace &elf->sechdrs[i] with &sechdrs[i] to slightly shorten
the code because we already have the local variable 'sechdrs'.

However, defining 'sechdr' instead shortens the code further.

Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>
---

 scripts/mod/modpost.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
index 99476a9695c5..441d57ee3275 100644
--- a/scripts/mod/modpost.c
+++ b/scripts/mod/modpost.c
@@ -1518,16 +1518,17 @@ static void section_rel(struct module *mod, struct elf_info *elf,
 static void check_sec_ref(struct module *mod, struct elf_info *elf)
 {
 	int i;
-	Elf_Shdr *sechdrs = elf->sechdrs;
 
 	/* Walk through all sections */
 	for (i = 0; i < elf->num_sections; i++) {
-		check_section(mod->name, elf, &elf->sechdrs[i]);
+		Elf_Shdr *sechdr = &elf->sechdrs[i];
+
+		check_section(mod->name, elf, sechdr);
 		/* We want to process only relocation sections and not .init */
-		if (sechdrs[i].sh_type == SHT_RELA)
-			section_rela(mod, elf, &elf->sechdrs[i]);
-		else if (sechdrs[i].sh_type == SHT_REL)
-			section_rel(mod, elf, &elf->sechdrs[i]);
+		if (sechdr->sh_type == SHT_RELA)
+			section_rela(mod, elf, sechdr);
+		else if (sechdr->sh_type == SHT_REL)
+			section_rel(mod, elf, sechdr);
 	}
 }
 
-- 
2.39.2

[PATCH 5/5] modpost: factor out the common boilerplate of section_rel(a)

[Masahiro Yamada]

The first few lines of section_rel() and section_rela() are the same.
They both retrieve the index of the section to which the relocaton
applies, and skip known-good sections. This common code should be moved
to check_sec_ref().

Avoid ugly casts when computing 'start' and 'stop', and also make the
Elf_Rel and Elf_Rela pointers const.

Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>
---

 scripts/mod/modpost.c | 50 ++++++++++++++++++++++---------------------
 1 file changed, 26 insertions(+), 24 deletions(-)

diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
index 441d57ee3275..f1f658122ad8 100644
--- a/scripts/mod/modpost.c
+++ b/scripts/mod/modpost.c
@@ -1420,17 +1420,10 @@ static void get_rel_type_and_sym(struct elf_info *elf, uint64_t r_info,
 }
 
 static void section_rela(struct module *mod, struct elf_info *elf,
-			 Elf_Shdr *sechdr)
+			 unsigned int fsecndx, const char *fromsec,
+			 const Elf_Rela *start, const Elf_Rela *stop)
 {
-	Elf_Rela *rela;
-	unsigned int fsecndx = sechdr->sh_info;
-	const char *fromsec = sec_name(elf, fsecndx);
-	Elf_Rela *start = (void *)elf->hdr + sechdr->sh_offset;
-	Elf_Rela *stop  = (void *)start + sechdr->sh_size;
-
-	/* if from section (name) is know good then skip it */
-	if (match(fromsec, section_white_list))
-		return;
+	const Elf_Rela *rela;
 
 	for (rela = start; rela < stop; rela++) {
 		Elf_Addr taddr, r_offset;
@@ -1460,17 +1453,10 @@ static void section_rela(struct module *mod, struct elf_info *elf,
 }
 
 static void section_rel(struct module *mod, struct elf_info *elf,
-			Elf_Shdr *sechdr)
+			unsigned int fsecndx, const char *fromsec,
+			const Elf_Rel *start, const Elf_Rel *stop)
 {
-	Elf_Rel *rel;
-	unsigned int fsecndx = sechdr->sh_info;
-	const char *fromsec = sec_name(elf, fsecndx);
-	Elf_Rel *start = (void *)elf->hdr + sechdr->sh_offset;
-	Elf_Rel *stop  = (void *)start + sechdr->sh_size;
-
-	/* if from section (name) is know good then skip it */
-	if (match(fromsec, section_white_list))
-		return;
+	const Elf_Rel *rel;
 
 	for (rel = start; rel < stop; rel++) {
 		Elf_Sym *tsym;
@@ -1525,10 +1511,26 @@ static void check_sec_ref(struct module *mod, struct elf_info *elf)
 
 		check_section(mod->name, elf, sechdr);
 		/* We want to process only relocation sections and not .init */
-		if (sechdr->sh_type == SHT_RELA)
-			section_rela(mod, elf, sechdr);
-		else if (sechdr->sh_type == SHT_REL)
-			section_rel(mod, elf, sechdr);
+		if (sechdr->sh_type == SHT_REL || sechdr->sh_type == SHT_RELA) {
+			/* section to which the relocation applies */
+			unsigned int secndx = sechdr->sh_info;
+			const char *secname = sec_name(elf, secndx);
+			const void *start, *stop;
+
+			/* If the section is known good, skip it */
+			if (match(secname, section_white_list))
+				continue;
+
+			start = sym_get_data_by_offset(elf, i, 0);
+			stop = start + sechdr->sh_size;
+
+			if (sechdr->sh_type == SHT_RELA)
+				section_rela(mod, elf, secndx, secname,
+					     start, stop);
+			else
+				section_rel(mod, elf, secndx, secname,
+					    start, stop);
+		}
 	}
 }
 
-- 
2.39.2

Re: [PATCH 2/5] modpost: fix ishtp MODULE_DEVICE_TABLE built on big endian host

[Thomas Weißschuh]

On 2023-10-08 02:04:45+0900, Masahiro Yamada wrote:
> When MODULE_DEVICE_TABLE(ishtp, ) is built on a host with a different
> endianness from the target architecture, it results in an incorrect
> MODULE_ALIAS().
> 
> For example, see a case where drivers/platform/x86/intel/ishtp_eclite.c
> is built as a module.

Nitpick:

... [as a module] for x86.

So the statements below can be interpreted correctly.

> 
> If you build it on a little endian host, you will get the correct
> MODULE_ALIAS:
> 
>     $ grep MODULE_ALIAS drivers/platform/x86/intel/ishtp_eclite.mod.c
>     MODULE_ALIAS("ishtp:{6A19CC4B-D760-4DE3-B14D-F25EBD0FBCD9}");
> 
> However, if you build it on a big endian host, you will get a wrong
> MODULE_ALIAS:
> 
>     $ grep MODULE_ALIAS drivers/platform/x86/intel/ishtp_eclite.mod.c
>     MODULE_ALIAS("ishtp:{BD0FBCD9-F25E-B14D-4DE3-D7606A19CC4B}");
> 
> This issue has been unnoticed because the x86 kernel is most likely built
> natively on an x86 host.
> 
> The guid field must not be reversed because guid_t is an array of __u8.
> 
> Fixes: fa443bc3c1e4 ("HID: intel-ish-hid: add support for MODULE_DEVICE_TABLE()")

+ Cc: stable@vger.kernel.org

> Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>

Reviewed-by: Thomas Weißschuh <linux@weissschuh.net>

Thanks!

> ---
> 
>  scripts/mod/file2alias.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/scripts/mod/file2alias.c b/scripts/mod/file2alias.c
> index 70bf6a2f585c..6583b36dbe69 100644
> --- a/scripts/mod/file2alias.c
> +++ b/scripts/mod/file2alias.c
> @@ -1401,10 +1401,10 @@ static int do_mhi_ep_entry(const char *filename, void *symval, char *alias)
>  /* Looks like: ishtp:{guid} */
>  static int do_ishtp_entry(const char *filename, void *symval, char *alias)
>  {
> -	DEF_FIELD(symval, ishtp_device_id, guid);
> +	DEF_FIELD_ADDR(symval, ishtp_device_id, guid);
>  
>  	strcpy(alias, ISHTP_MODULE_PREFIX "{");
> -	add_guid(alias, guid);
> +	add_guid(alias, *guid);
>  	strcat(alias, "}");
>  
>  	return 1;
> -- 
> 2.39.2
> 

Re: [PATCH 1/5] modpost: fix tee MODULE_DEVICE_TABLE built on big endian host

[Sumit Garg]

Hi Masahiro,

On Sat, 7 Oct 2023 at 22:34, Masahiro Yamada <masahiroy@kernel.org> wrote:
>
> When MODULE_DEVICE_TABLE(tee, ) is built on a host with a different
> endianness from the target architecture, it results in an incorrect
> MODULE_ALIAS().
>
> For example, see a case where drivers/char/hw_random/optee-rng.c
> is built as a module.
>
> If you build it on a little endian host, you will get the correct
> MODULE_ALIAS:
>
>     $ grep MODULE_ALIAS drivers/char/hw_random/optee-rng.mod.c
>     MODULE_ALIAS("tee:ab7a617c-b8e7-4d8f-8301-d09b61036b64*");
>
> However, if you build it on a big endian host, you will get a wrong
> MODULE_ALIAS:
>
>     $ grep MODULE_ALIAS drivers/char/hw_random/optee-rng.mod.c
>     MODULE_ALIAS("tee:646b0361-9bd0-0183-8f4d-e7b87c617aab*");
>
> This issue has been unnoticed because the ARM kernel is most likely built
> on a little endian host (cross-build on x86 or native-build on ARM).
>
> The uuid field must not be reversed because uuid_t is an array of __u8.
>

To me it wasn't obvious that DEF_FIELD() has certain endianness limitations.

> Fixes: 0fc1db9d1059 ("tee: add bus driver framework for TEE based devices")
> Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
> ---
>
>  scripts/mod/file2alias.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/scripts/mod/file2alias.c b/scripts/mod/file2alias.c
> index 7056751c29b1..70bf6a2f585c 100644
> --- a/scripts/mod/file2alias.c
> +++ b/scripts/mod/file2alias.c
> @@ -1348,13 +1348,13 @@ static int do_typec_entry(const char *filename, void *symval, char *alias)
>  /* Looks like: tee:uuid */
>  static int do_tee_entry(const char *filename, void *symval, char *alias)
>  {
> -       DEF_FIELD(symval, tee_client_device_id, uuid);

As you have mentioned in patch #3: the limitations of TO_NATIVE(), if
you can update comments for DEF_FIELD() as well to make it clear that
it doesn't support byte arrays/strings would be helpful. I think the
following check that you have introduced in patch #3 can still be
bypassed for byte arrays/strings.

+ _Static_assert(sizeof(x) == 1 || sizeof(x) == 2 || \
+       sizeof(x) == 4 || sizeof(x) == 8, "bug");

BTW, for this fix feel free to add:

Reviewed-by: Sumit Garg <sumit.garg@linaro.org>

-Sumit

> +       DEF_FIELD_ADDR(symval, tee_client_device_id, uuid);
>
>         sprintf(alias, "tee:%02x%02x%02x%02x-%02x%02x-%02x%02x-%02x%02x-%02x%02x%02x%02x%02x%02x",
> -               uuid.b[0], uuid.b[1], uuid.b[2], uuid.b[3], uuid.b[4],
> -               uuid.b[5], uuid.b[6], uuid.b[7], uuid.b[8], uuid.b[9],
> -               uuid.b[10], uuid.b[11], uuid.b[12], uuid.b[13], uuid.b[14],
> -               uuid.b[15]);
> +               uuid->b[0], uuid->b[1], uuid->b[2], uuid->b[3], uuid->b[4],
> +               uuid->b[5], uuid->b[6], uuid->b[7], uuid->b[8], uuid->b[9],
> +               uuid->b[10], uuid->b[11], uuid->b[12], uuid->b[13], uuid->b[14],
> +               uuid->b[15]);
>
>         add_wildcard(alias);
>         return 1;
> --
> 2.39.2
>

Re: [PATCH 1/5] modpost: fix tee MODULE_DEVICE_TABLE built on big endian host

[Masahiro Yamada]

On Mon, Oct 9, 2023 at 3:27 PM Sumit Garg <sumit.garg@linaro.org> wrote:
>
> Hi Masahiro,
>
> On Sat, 7 Oct 2023 at 22:34, Masahiro Yamada <masahiroy@kernel.org> wrote:
> >
> > When MODULE_DEVICE_TABLE(tee, ) is built on a host with a different
> > endianness from the target architecture, it results in an incorrect
> > MODULE_ALIAS().
> >
> > For example, see a case where drivers/char/hw_random/optee-rng.c
> > is built as a module.
> >
> > If you build it on a little endian host, you will get the correct
> > MODULE_ALIAS:
> >
> >     $ grep MODULE_ALIAS drivers/char/hw_random/optee-rng.mod.c
> >     MODULE_ALIAS("tee:ab7a617c-b8e7-4d8f-8301-d09b61036b64*");
> >
> > However, if you build it on a big endian host, you will get a wrong
> > MODULE_ALIAS:
> >
> >     $ grep MODULE_ALIAS drivers/char/hw_random/optee-rng.mod.c
> >     MODULE_ALIAS("tee:646b0361-9bd0-0183-8f4d-e7b87c617aab*");
> >
> > This issue has been unnoticed because the ARM kernel is most likely built
> > on a little endian host (cross-build on x86 or native-build on ARM).
> >
> > The uuid field must not be reversed because uuid_t is an array of __u8.
> >
>
> To me it wasn't obvious that DEF_FIELD() has certain endianness limitations.
>
> > Fixes: 0fc1db9d1059 ("tee: add bus driver framework for TEE based devices")
> > Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
> > ---
> >
> >  scripts/mod/file2alias.c | 10 +++++-----
> >  1 file changed, 5 insertions(+), 5 deletions(-)
> >
> > diff --git a/scripts/mod/file2alias.c b/scripts/mod/file2alias.c
> > index 7056751c29b1..70bf6a2f585c 100644
> > --- a/scripts/mod/file2alias.c
> > +++ b/scripts/mod/file2alias.c
> > @@ -1348,13 +1348,13 @@ static int do_typec_entry(const char *filename, void *symval, char *alias)
> >  /* Looks like: tee:uuid */
> >  static int do_tee_entry(const char *filename, void *symval, char *alias)
> >  {
> > -       DEF_FIELD(symval, tee_client_device_id, uuid);
>
> As you have mentioned in patch #3: the limitations of TO_NATIVE(), if
> you can update comments for DEF_FIELD() as well to make it clear that
> it doesn't support byte arrays/strings would be helpful. I think the
> following check that you have introduced in patch #3 can still be
> bypassed for byte arrays/strings.
>
> + _Static_assert(sizeof(x) == 1 || sizeof(x) == 2 || \
> +       sizeof(x) == 4 || sizeof(x) == 8, "bug");




I am afraid you missed the point.

bswap_2, bswap_4, bswap_8 do not take a pointer.

If you pass an array or a string,
it will result in a build error
due to the compiler's prototype checking.

The kbuild test robot will catch a build error anyway.


"You cannot build it in the first place"
is better than a comment.










> BTW, for this fix feel free to add:
>
> Reviewed-by: Sumit Garg <sumit.garg@linaro.org>
>
> -Sumit
>
> > +       DEF_FIELD_ADDR(symval, tee_client_device_id, uuid);
> >
> >         sprintf(alias, "tee:%02x%02x%02x%02x-%02x%02x-%02x%02x-%02x%02x-%02x%02x%02x%02x%02x%02x",
> > -               uuid.b[0], uuid.b[1], uuid.b[2], uuid.b[3], uuid.b[4],
> > -               uuid.b[5], uuid.b[6], uuid.b[7], uuid.b[8], uuid.b[9],
> > -               uuid.b[10], uuid.b[11], uuid.b[12], uuid.b[13], uuid.b[14],
> > -               uuid.b[15]);
> > +               uuid->b[0], uuid->b[1], uuid->b[2], uuid->b[3], uuid->b[4],
> > +               uuid->b[5], uuid->b[6], uuid->b[7], uuid->b[8], uuid->b[9],
> > +               uuid->b[10], uuid->b[11], uuid->b[12], uuid->b[13], uuid->b[14],
> > +               uuid->b[15]);
> >
> >         add_wildcard(alias);
> >         return 1;
> > --
> > 2.39.2
> >



--
Best Regards
Masahiro Yamada

Re: [PATCH 1/5] modpost: fix tee MODULE_DEVICE_TABLE built on big endian host

[Sumit Garg]

On Mon, 9 Oct 2023 at 12:27, Masahiro Yamada <masahiroy@kernel.org> wrote:
>
> On Mon, Oct 9, 2023 at 3:27 PM Sumit Garg <sumit.garg@linaro.org> wrote:
> >
> > Hi Masahiro,
> >
> > On Sat, 7 Oct 2023 at 22:34, Masahiro Yamada <masahiroy@kernel.org> wrote:
> > >
> > > When MODULE_DEVICE_TABLE(tee, ) is built on a host with a different
> > > endianness from the target architecture, it results in an incorrect
> > > MODULE_ALIAS().
> > >
> > > For example, see a case where drivers/char/hw_random/optee-rng.c
> > > is built as a module.
> > >
> > > If you build it on a little endian host, you will get the correct
> > > MODULE_ALIAS:
> > >
> > >     $ grep MODULE_ALIAS drivers/char/hw_random/optee-rng.mod.c
> > >     MODULE_ALIAS("tee:ab7a617c-b8e7-4d8f-8301-d09b61036b64*");
> > >
> > > However, if you build it on a big endian host, you will get a wrong
> > > MODULE_ALIAS:
> > >
> > >     $ grep MODULE_ALIAS drivers/char/hw_random/optee-rng.mod.c
> > >     MODULE_ALIAS("tee:646b0361-9bd0-0183-8f4d-e7b87c617aab*");
> > >
> > > This issue has been unnoticed because the ARM kernel is most likely built
> > > on a little endian host (cross-build on x86 or native-build on ARM).
> > >
> > > The uuid field must not be reversed because uuid_t is an array of __u8.
> > >
> >
> > To me it wasn't obvious that DEF_FIELD() has certain endianness limitations.
> >
> > > Fixes: 0fc1db9d1059 ("tee: add bus driver framework for TEE based devices")
> > > Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
> > > ---
> > >
> > >  scripts/mod/file2alias.c | 10 +++++-----
> > >  1 file changed, 5 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/scripts/mod/file2alias.c b/scripts/mod/file2alias.c
> > > index 7056751c29b1..70bf6a2f585c 100644
> > > --- a/scripts/mod/file2alias.c
> > > +++ b/scripts/mod/file2alias.c
> > > @@ -1348,13 +1348,13 @@ static int do_typec_entry(const char *filename, void *symval, char *alias)
> > >  /* Looks like: tee:uuid */
> > >  static int do_tee_entry(const char *filename, void *symval, char *alias)
> > >  {
> > > -       DEF_FIELD(symval, tee_client_device_id, uuid);
> >
> > As you have mentioned in patch #3: the limitations of TO_NATIVE(), if
> > you can update comments for DEF_FIELD() as well to make it clear that
> > it doesn't support byte arrays/strings would be helpful. I think the
> > following check that you have introduced in patch #3 can still be
> > bypassed for byte arrays/strings.
> >
> > + _Static_assert(sizeof(x) == 1 || sizeof(x) == 2 || \
> > +       sizeof(x) == 4 || sizeof(x) == 8, "bug");
>
>
>
>
> I am afraid you missed the point.
>
> bswap_2, bswap_4, bswap_8 do not take a pointer.
>
> If you pass an array or a string,
> it will result in a build error
> due to the compiler's prototype checking.
>
> The kbuild test robot will catch a build error anyway.
>

I see your point.

>
> "You cannot build it in the first place"
> is better than a comment.
>

That's fine with me as long as it's a build problem.

-Sumit

>
>
>
>
>
>
>
>
>
> > BTW, for this fix feel free to add:
> >
> > Reviewed-by: Sumit Garg <sumit.garg@linaro.org>
> >
> > -Sumit
> >
> > > +       DEF_FIELD_ADDR(symval, tee_client_device_id, uuid);
> > >
> > >         sprintf(alias, "tee:%02x%02x%02x%02x-%02x%02x-%02x%02x-%02x%02x-%02x%02x%02x%02x%02x%02x",
> > > -               uuid.b[0], uuid.b[1], uuid.b[2], uuid.b[3], uuid.b[4],
> > > -               uuid.b[5], uuid.b[6], uuid.b[7], uuid.b[8], uuid.b[9],
> > > -               uuid.b[10], uuid.b[11], uuid.b[12], uuid.b[13], uuid.b[14],
> > > -               uuid.b[15]);
> > > +               uuid->b[0], uuid->b[1], uuid->b[2], uuid->b[3], uuid->b[4],
> > > +               uuid->b[5], uuid->b[6], uuid->b[7], uuid->b[8], uuid->b[9],
> > > +               uuid->b[10], uuid->b[11], uuid->b[12], uuid->b[13], uuid->b[14],
> > > +               uuid->b[15]);
> > >
> > >         add_wildcard(alias);
> > >         return 1;
> > > --
> > > 2.39.2
> > >
>
>
>
> --
> Best Regards
> Masahiro Yamada

Re: [PATCH 2/5] modpost: fix ishtp MODULE_DEVICE_TABLE built on big endian host

[srinivas pandruvada]

On Sun, 2023-10-08 at 09:51 +0200, Thomas Weißschuh wrote:
> On 2023-10-08 02:04:45+0900, Masahiro Yamada wrote:
> > When MODULE_DEVICE_TABLE(ishtp, ) is built on a host with a
> > different
> > endianness from the target architecture, it results in an incorrect
> > MODULE_ALIAS().
> > 
> > For example, see a case where
> > drivers/platform/x86/intel/ishtp_eclite.c
> > is built as a module.
> 
> Nitpick:
> 
> ... [as a module] for x86.
> 
> So the statements below can be interpreted correctly.
> 
> > 
> > If you build it on a little endian host, you will get the correct
> > MODULE_ALIAS:
> > 
> >     $ grep MODULE_ALIAS
> > drivers/platform/x86/intel/ishtp_eclite.mod.c
> >     MODULE_ALIAS("ishtp:{6A19CC4B-D760-4DE3-B14D-F25EBD0FBCD9}");
> > 
> > However, if you build it on a big endian host, you will get a wrong
> > MODULE_ALIAS:
> > 
> >     $ grep MODULE_ALIAS
> > drivers/platform/x86/intel/ishtp_eclite.mod.c
> >     MODULE_ALIAS("ishtp:{BD0FBCD9-F25E-B14D-4DE3-D7606A19CC4B}");
> > 
> > This issue has been unnoticed because the x86 kernel is most likely
> > built
> > natively on an x86 host.
> > 
> > The guid field must not be reversed because guid_t is an array of
> > __u8.
> > 
> > Fixes: fa443bc3c1e4 ("HID: intel-ish-hid: add support for
> > MODULE_DEVICE_TABLE()")
> 
> + Cc: stable@vger.kernel.org
> 
> > Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
> 
> Reviewed-by: Thomas Weißschuh <linux@weissschuh.net>

Tested-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>

Acked-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>

Thanks,
Srinivas

> 
> Thanks!
> 
> > ---
> > 
> >  scripts/mod/file2alias.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/scripts/mod/file2alias.c b/scripts/mod/file2alias.c
> > index 70bf6a2f585c..6583b36dbe69 100644
> > --- a/scripts/mod/file2alias.c
> > +++ b/scripts/mod/file2alias.c
> > @@ -1401,10 +1401,10 @@ static int do_mhi_ep_entry(const char
> > *filename, void *symval, char *alias)
> >  /* Looks like: ishtp:{guid} */
> >  static int do_ishtp_entry(const char *filename, void *symval, char
> > *alias)
> >  {
> > -       DEF_FIELD(symval, ishtp_device_id, guid);
> > +       DEF_FIELD_ADDR(symval, ishtp_device_id, guid);
> >  
> >         strcpy(alias, ISHTP_MODULE_PREFIX "{");
> > -       add_guid(alias, guid);
> > +       add_guid(alias, *guid);
> >         strcat(alias, "}");
> >  
> >         return 1;
> > -- 
> > 2.39.2
> > 

Re: [PATCH 3/5] modpost: define TO_NATIVE() using bswap_* functions

[Nick Desaulniers]

On Sat, Oct 7, 2023 at 10:05 AM Masahiro Yamada <masahiroy@kernel.org> wrote:
>
> The current TO_NATIVE() has some limitations:
>
>  1) You cannot cast the argument.
>
>  2) You cannot pass a variable marked as 'const'.
>
>  3) Passing an array is a bug, but it is not detected.
>
> Impelement TO_NATIVE() using bswap_*() functions. These are GNU
> extensions. If we face portability issues, we can port the code from
> include/uapi/linux/swab.h.
>
> With this change, get_rel_type_and_sym() can be simplified by casting
> the arguments directly.
>
> Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
> ---
>
>  scripts/mod/modpost.c | 13 ++++---------
>  scripts/mod/modpost.h | 25 ++++++++++++-------------
>  2 files changed, 16 insertions(+), 22 deletions(-)
>
> diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
> index 2f3b0fe6f68d..99476a9695c5 100644
> --- a/scripts/mod/modpost.c
> +++ b/scripts/mod/modpost.c
> @@ -1410,15 +1410,10 @@ static void get_rel_type_and_sym(struct elf_info *elf, uint64_t r_info,
>                 return;
>         }
>
> -       if (is_64bit) {
> -               Elf64_Xword r_info64 = r_info;
> -
> -               r_info = TO_NATIVE(r_info64);
> -       } else {
> -               Elf32_Word r_info32 = r_info;
> -
> -               r_info = TO_NATIVE(r_info32);
> -       }
> +       if (is_64bit)
> +               r_info = TO_NATIVE((Elf64_Xword)r_info);
> +       else
> +               r_info = TO_NATIVE((Elf32_Word)r_info);
>
>         *r_type = ELF_R_TYPE(r_info);
>         *r_sym = ELF_R_SYM(r_info);
> diff --git a/scripts/mod/modpost.h b/scripts/mod/modpost.h
> index 6413f26fcb6b..1392afec118c 100644
> --- a/scripts/mod/modpost.h
> +++ b/scripts/mod/modpost.h
> @@ -1,4 +1,5 @@
>  /* SPDX-License-Identifier: GPL-2.0 */
> +#include <byteswap.h>
>  #include <stdbool.h>
>  #include <stdio.h>
>  #include <stdlib.h>
> @@ -51,21 +52,19 @@
>  #define ELF_R_TYPE  ELF64_R_TYPE
>  #endif
>
> +#define bswap(x) \
> +({ \
> +       _Static_assert(sizeof(x) == 1 || sizeof(x) == 2 || \

Seems fine, but do we need to support folks trying to swap 1B values?
i.e. is someone calling TO_NATIVE with 1B values?  Seems silly unless
one of these types is variable length dependent on the target machine
type?

Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>

> +                      sizeof(x) == 4 || sizeof(x) == 8, "bug"); \
> +       (typeof(x))(sizeof(x) == 2 ? bswap_16(x) : \
> +                   sizeof(x) == 4 ? bswap_32(x) : \
> +                   sizeof(x) == 8 ? bswap_64(x) : \
> +                   x); \
> +})
> +
>  #if KERNEL_ELFDATA != HOST_ELFDATA
>
> -static inline void __endian(const void *src, void *dest, unsigned int size)
> -{
> -       unsigned int i;
> -       for (i = 0; i < size; i++)
> -               ((unsigned char*)dest)[i] = ((unsigned char*)src)[size - i-1];
> -}
> -
> -#define TO_NATIVE(x)                                           \
> -({                                                             \
> -       typeof(x) __x;                                          \
> -       __endian(&(x), &(__x), sizeof(__x));                    \
> -       __x;                                                    \
> -})
> +#define TO_NATIVE(x) (bswap(x))
>
>  #else /* endianness matches */
>
> --
> 2.39.2
>


-- 
Thanks,
~Nick Desaulniers

Re: [PATCH 3/5] modpost: define TO_NATIVE() using bswap_* functions

[Masahiro Yamada]

On Tue, Oct 10, 2023 at 2:44 AM Nick Desaulniers
<ndesaulniers@google.com> wrote:
>
> On Sat, Oct 7, 2023 at 10:05 AM Masahiro Yamada <masahiroy@kernel.org> wrote:
> >
> > The current TO_NATIVE() has some limitations:
> >
> >  1) You cannot cast the argument.
> >
> >  2) You cannot pass a variable marked as 'const'.
> >
> >  3) Passing an array is a bug, but it is not detected.
> >
> > Impelement TO_NATIVE() using bswap_*() functions. These are GNU
> > extensions. If we face portability issues, we can port the code from
> > include/uapi/linux/swab.h.
> >
> > With this change, get_rel_type_and_sym() can be simplified by casting
> > the arguments directly.
> >
> > Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
> > ---
> >
> >  scripts/mod/modpost.c | 13 ++++---------
> >  scripts/mod/modpost.h | 25 ++++++++++++-------------
> >  2 files changed, 16 insertions(+), 22 deletions(-)
> >
> > diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
> > index 2f3b0fe6f68d..99476a9695c5 100644
> > --- a/scripts/mod/modpost.c
> > +++ b/scripts/mod/modpost.c
> > @@ -1410,15 +1410,10 @@ static void get_rel_type_and_sym(struct elf_info *elf, uint64_t r_info,
> >                 return;
> >         }
> >
> > -       if (is_64bit) {
> > -               Elf64_Xword r_info64 = r_info;
> > -
> > -               r_info = TO_NATIVE(r_info64);
> > -       } else {
> > -               Elf32_Word r_info32 = r_info;
> > -
> > -               r_info = TO_NATIVE(r_info32);
> > -       }
> > +       if (is_64bit)
> > +               r_info = TO_NATIVE((Elf64_Xword)r_info);
> > +       else
> > +               r_info = TO_NATIVE((Elf32_Word)r_info);
> >
> >         *r_type = ELF_R_TYPE(r_info);
> >         *r_sym = ELF_R_SYM(r_info);
> > diff --git a/scripts/mod/modpost.h b/scripts/mod/modpost.h
> > index 6413f26fcb6b..1392afec118c 100644
> > --- a/scripts/mod/modpost.h
> > +++ b/scripts/mod/modpost.h
> > @@ -1,4 +1,5 @@
> >  /* SPDX-License-Identifier: GPL-2.0 */
> > +#include <byteswap.h>
> >  #include <stdbool.h>
> >  #include <stdio.h>
> >  #include <stdlib.h>
> > @@ -51,21 +52,19 @@
> >  #define ELF_R_TYPE  ELF64_R_TYPE
> >  #endif
> >
> > +#define bswap(x) \
> > +({ \
> > +       _Static_assert(sizeof(x) == 1 || sizeof(x) == 2 || \
>
> Seems fine, but do we need to support folks trying to swap 1B values?
> i.e. is someone calling TO_NATIVE with 1B values?



Yes.

In scripts/mod/file2alias.c,
DEF_FIELD() calls TO_NATIVE().

DEF_FIELD() is also used to get access to 1-byte fields.
So, TO_NATIVE() needs to accept sizeof(x)==1.



>  Seems silly unless
> one of these types is variable length dependent on the target machine
> type?


You can use DEF_FIELD() without knowing the
field width. This is good.





-- 
Best Regards
Masahiro Yamada

Re: [PATCH 2/5] modpost: fix ishtp MODULE_DEVICE_TABLE built on big endian host

[Masahiro Yamada]

On Sun, Oct 8, 2023 at 4:51 PM Thomas Weißschuh <linux@weissschuh.net> wrote:
>
> On 2023-10-08 02:04:45+0900, Masahiro Yamada wrote:
> > When MODULE_DEVICE_TABLE(ishtp, ) is built on a host with a different
> > endianness from the target architecture, it results in an incorrect
> > MODULE_ALIAS().
> >
> > For example, see a case where drivers/platform/x86/intel/ishtp_eclite.c
> > is built as a module.
>
> Nitpick:
>
> ... [as a module] for x86.
>
> So the statements below can be interpreted correctly.


OK, I fixed up the comment when I applied the patch.
Thanks.


-- 
Best Regards
Masahiro Yamada