[PATCH] mkimage: fix segfault on MacOS arm64

[PATCH] mkimage: fix segfault on MacOS arm64

[Sergey V. Lobanov]

mkimage segfaults due ASLR mechasim on MacOS arm64

It is required to use _dyld_get_image_vmaddr_slide()
to prevent segfault on MacOS arm64

This patch ased on the discussion
https://github.com/u-boot/u-boot/commit/3b142045e8a7f0ab17b6099e9226296af45967d0

Thanks to Ronny Kotzschmar and ptpt52 github user

Signed-off-by: Sergey V. Lobanov <sergey@lobanov.in>
---
 tools/imagetool.h | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/tools/imagetool.h b/tools/imagetool.h
index e229a34ffc..13775ff9b3 100644
--- a/tools/imagetool.h
+++ b/tools/imagetool.h
@@ -271,11 +271,16 @@ int rockchip_copy_image(int fd, struct image_tool_params *mparams);
  *  b) we need a API call to get the respective section symbols */
 #if defined(__MACH__)
 #include <mach-o/getsect.h>
+#include <mach-o/dyld.h>
 
-#define INIT_SECTION(name)  do {					\
+#define INIT_SECTION(name)	struct image_type_params		\
+	**__cat(__start_, name), **__cat(__stop_, name);		\
+	do {								\
 		unsigned long name ## _len;				\
 		char *__cat(pstart_, name) = getsectdata("__DATA",	\
 			#name, &__cat(name, _len));			\
+			__cat(pstart_, name) +=				\
+				_dyld_get_image_vmaddr_slide(0);	\
 		char *__cat(pstop_, name) = __cat(pstart_, name) +	\
 			__cat(name, _len);				\
 		__cat(__start_, name) = (void *)__cat(pstart_, name);	\
@@ -283,7 +288,6 @@ int rockchip_copy_image(int fd, struct image_tool_params *mparams);
 	} while (0)
 #define SECTION(name)   __attribute__((section("__DATA, " #name)))
 
-struct image_type_params **__start_image_type, **__stop_image_type;
 #else
 #define INIT_SECTION(name) /* no-op for ELF */
 #define SECTION(name)   __attribute__((section(#name)))
-- 
2.30.1 (Apple Git-130)

Re: [PATCH] mkimage: fix segfault on MacOS arm64

[Sergey V. Lobanov]

Hello,

Can someone please review this?

> On 2 Dec 2021, at 22:16, Sergey V. Lobanov <sergey@lobanov.in> wrote:
> 
> mkimage segfaults due ASLR mechasim on MacOS arm64
> 
> It is required to use _dyld_get_image_vmaddr_slide()
> to prevent segfault on MacOS arm64
> 
> This patch ased on the discussion
> https://github.com/u-boot/u-boot/commit/3b142045e8a7f0ab17b6099e9226296af45967d0
> 
> Thanks to Ronny Kotzschmar and ptpt52 github user
> 
> Signed-off-by: Sergey V. Lobanov <sergey@lobanov.in>
> ---
> tools/imagetool.h | 8 ++++++--
> 1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/imagetool.h b/tools/imagetool.h
> index e229a34ffc..13775ff9b3 100644
> --- a/tools/imagetool.h
> +++ b/tools/imagetool.h
> @@ -271,11 +271,16 @@ int rockchip_copy_image(int fd, struct image_tool_params *mparams);
>  *  b) we need a API call to get the respective section symbols */
> #if defined(__MACH__)
> #include <mach-o/getsect.h>
> +#include <mach-o/dyld.h>
> 
> -#define INIT_SECTION(name)  do {					\
> +#define INIT_SECTION(name)	struct image_type_params		\
> +	**__cat(__start_, name), **__cat(__stop_, name);		\
> +	do {								\
> 		unsigned long name ## _len;				\
> 		char *__cat(pstart_, name) = getsectdata("__DATA",	\
> 			#name, &__cat(name, _len));			\
> +			__cat(pstart_, name) +=				\
> +				_dyld_get_image_vmaddr_slide(0);	\
> 		char *__cat(pstop_, name) = __cat(pstart_, name) +	\
> 			__cat(name, _len);				\
> 		__cat(__start_, name) = (void *)__cat(pstart_, name);	\
> @@ -283,7 +288,6 @@ int rockchip_copy_image(int fd, struct image_tool_params *mparams);
> 	} while (0)
> #define SECTION(name)   __attribute__((section("__DATA, " #name)))
> 
> -struct image_type_params **__start_image_type, **__stop_image_type;
> #else
> #define INIT_SECTION(name) /* no-op for ELF */
> #define SECTION(name)   __attribute__((section(#name)))
> -- 
> 2.30.1 (Apple Git-130)
> 

Re: [PATCH] mkimage: fix segfault on MacOS arm64

[Jessica Clarke]

On 2 Dec 2021, at 22:16, Sergey V. Lobanov <sergey@lobanov.in> wrote:
> 
> mkimage segfaults due ASLR mechasim on MacOS arm64
> 
> It is required to use _dyld_get_image_vmaddr_slide()
> to prevent segfault on MacOS arm64
> 
> This patch ased on the discussion
> https://github.com/u-boot/u-boot/commit/3b142045e8a7f0ab17b6099e9226296af45967d0
> 
> Thanks to Ronny Kotzschmar and ptpt52 github user
> 
> Signed-off-by: Sergey V. Lobanov <sergey@lobanov.in>
> ---
> tools/imagetool.h | 8 ++++++--
> 1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/imagetool.h b/tools/imagetool.h
> index e229a34ffc..13775ff9b3 100644
> --- a/tools/imagetool.h
> +++ b/tools/imagetool.h
> @@ -271,11 +271,16 @@ int rockchip_copy_image(int fd, struct image_tool_params *mparams);
> *  b) we need a API call to get the respective section symbols */
> #if defined(__MACH__)
> #include <mach-o/getsect.h>
> +#include <mach-o/dyld.h>
> 
> -#define INIT_SECTION(name)  do {					\
> +#define INIT_SECTION(name)	struct image_type_params		\
> +	**__cat(__start_, name), **__cat(__stop_, name);		\

This change alters the interface of INIT_SECTION. Previously it was
just required that something had called it before you referenced the
start/stop symbols. Now there are two things going on:

1. Any references have to be in a scope that can see the INIT_SECTION
   call.
2. This is no longer a single statement, so
       if (foo)
           INIT_SECTION(name);
    breaks.

I don’t see why this change is needed, either. It should still be
idempotent to set the global multiple times even after your change to
add the base address, since that is done to the temporary local
variable.

> +	do {								\
> 		unsigned long name ## _len;				\
> 		char *__cat(pstart_, name) = getsectdata("__DATA",	\
> 			#name, &__cat(name, _len));			\
> +			__cat(pstart_, name) +=				\
> +				_dyld_get_image_vmaddr_slide(0);	\

Your formatting here is broken, you have an extra tab on both lines.

Jess

> 		char *__cat(pstop_, name) = __cat(pstart_, name) +	\
> 			__cat(name, _len);				\
> 		__cat(__start_, name) = (void *)__cat(pstart_, name);	\
> @@ -283,7 +288,6 @@ int rockchip_copy_image(int fd, struct image_tool_params *mparams);
> 	} while (0)
> #define SECTION(name)   __attribute__((section("__DATA, " #name)))
> 
> -struct image_type_params **__start_image_type, **__stop_image_type;
> #else
> #define INIT_SECTION(name) /* no-op for ELF */
> #define SECTION(name)   __attribute__((section(#name)))
> -- 
> 2.30.1 (Apple Git-130)

Re: [PATCH] mkimage: fix segfault on MacOS arm64

[Sergey V. Lobanov]

Thanks a lot for your review, I’ve sent PATCH v2 with the changes related to your comments

https://lists.denx.de/pipermail/u-boot/2022-January/472133.html


> On 11 Jan 2022, at 22:42, Jessica Clarke <jrtc27@jrtc27.com> wrote:
> 
> On 2 Dec 2021, at 22:16, Sergey V. Lobanov <sergey@lobanov.in> wrote:
>> 
>> mkimage segfaults due ASLR mechasim on MacOS arm64
>> 
>> It is required to use _dyld_get_image_vmaddr_slide()
>> to prevent segfault on MacOS arm64
>> 
>> This patch ased on the discussion
>> https://github.com/u-boot/u-boot/commit/3b142045e8a7f0ab17b6099e9226296af45967d0
>> 
>> Thanks to Ronny Kotzschmar and ptpt52 github user
>> 
>> Signed-off-by: Sergey V. Lobanov <sergey@lobanov.in>
>> ---
>> tools/imagetool.h | 8 ++++++--
>> 1 file changed, 6 insertions(+), 2 deletions(-)
>> 
>> diff --git a/tools/imagetool.h b/tools/imagetool.h
>> index e229a34ffc..13775ff9b3 100644
>> --- a/tools/imagetool.h
>> +++ b/tools/imagetool.h
>> @@ -271,11 +271,16 @@ int rockchip_copy_image(int fd, struct image_tool_params *mparams);
>> *  b) we need a API call to get the respective section symbols */
>> #if defined(__MACH__)
>> #include <mach-o/getsect.h>
>> +#include <mach-o/dyld.h>
>> 
>> -#define INIT_SECTION(name)  do {					\
>> +#define INIT_SECTION(name)	struct image_type_params		\
>> +	**__cat(__start_, name), **__cat(__stop_, name);		\
> 
> This change alters the interface of INIT_SECTION. Previously it was
> just required that something had called it before you referenced the
> start/stop symbols. Now there are two things going on:
> 
> 1. Any references have to be in a scope that can see the INIT_SECTION
>   call.
> 2. This is no longer a single statement, so
>       if (foo)
>           INIT_SECTION(name);
>    breaks.
> 
> I don’t see why this change is needed, either. It should still be
> idempotent to set the global multiple times even after your change to
> add the base address, since that is done to the temporary local
> variable.
> 
>> +	do {								\
>> 		unsigned long name ## _len;				\
>> 		char *__cat(pstart_, name) = getsectdata("__DATA",	\
>> 			#name, &__cat(name, _len));			\
>> +			__cat(pstart_, name) +=				\
>> +				_dyld_get_image_vmaddr_slide(0);	\
> 
> Your formatting here is broken, you have an extra tab on both lines.
> 
> Jess
> 
>> 		char *__cat(pstop_, name) = __cat(pstart_, name) +	\
>> 			__cat(name, _len);				\
>> 		__cat(__start_, name) = (void *)__cat(pstart_, name);	\
>> @@ -283,7 +288,6 @@ int rockchip_copy_image(int fd, struct image_tool_params *mparams);
>> 	} while (0)
>> #define SECTION(name)   __attribute__((section("__DATA, " #name)))
>> 
>> -struct image_type_params **__start_image_type, **__stop_image_type;
>> #else
>> #define INIT_SECTION(name) /* no-op for ELF */
>> #define SECTION(name)   __attribute__((section(#name)))
>> -- 
>> 2.30.1 (Apple Git-130)