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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
Reviewed-by: Nick Desaulniers <[email protected]>
---
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
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 <[email protected]>
Reviewed-by: Nick Desaulniers <[email protected]>
---
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
Hi Masahiro,
On Sat, 7 Oct 2023 at 22:34, Masahiro Yamada <[email protected]> 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 <[email protected]>
> ---
>
> 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 <[email protected]>
-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
>
On Mon, Oct 9, 2023 at 3:27 PM Sumit Garg <[email protected]> wrote:
>
> Hi Masahiro,
>
> On Sat, 7 Oct 2023 at 22:34, Masahiro Yamada <[email protected]> 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 <[email protected]>
> > ---
> >
> > 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 <[email protected]>
>
> -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
On Mon, 9 Oct 2023 at 12:27, Masahiro Yamada <[email protected]> wrote:
>
> On Mon, Oct 9, 2023 at 3:27 PM Sumit Garg <[email protected]> wrote:
> >
> > Hi Masahiro,
> >
> > On Sat, 7 Oct 2023 at 22:34, Masahiro Yamada <[email protected]> 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 <[email protected]>
> > > ---
> > >
> > > 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 <[email protected]>
> >
> > -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
On Sat, Oct 7, 2023 at 10:05 AM Masahiro Yamada <[email protected]> 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 <[email protected]>
> ---
>
> 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 <[email protected]>
> + 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
On Tue, Oct 10, 2023 at 2:44 AM Nick Desaulniers
<[email protected]> wrote:
>
> On Sat, Oct 7, 2023 at 10:05 AM Masahiro Yamada <[email protected]> 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 <[email protected]>
> > ---
> >
> > 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