* Re: [PATCH 1/1] linux-user/syscall: Fix do_ioctl_ifconf() for 64 bit targets.
2021-01-09 18:59 [PATCH 1/1] linux-user/syscall: Fix do_ioctl_ifconf() for 64 bit targets Stefan
@ 2021-01-17 13:00 ` Stefan
2021-01-18 14:03 ` Laurent Vivier
` (2 subsequent siblings)
3 siblings, 0 replies; 7+ messages in thread
From: Stefan @ 2021-01-17 13:00 UTC (permalink / raw)
To: qemu-devel, qemu-trivial; +Cc: Laurent Vivier, qemu-stable
ping
http://patchwork.ozlabs.org/project/qemu-devel/patch/60AA0765-53DD-43D1-A3D2-75F1778526F6@vodafonemail.de/
Hi!
I’d like to remind you to this trivial patch to get ioclt(…, SIOCGIFCONF, …) working properly on 64 bit target architectures.
Bye
Stefan
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH 1/1] linux-user/syscall: Fix do_ioctl_ifconf() for 64 bit targets.
2021-01-09 18:59 [PATCH 1/1] linux-user/syscall: Fix do_ioctl_ifconf() for 64 bit targets Stefan
2021-01-17 13:00 ` Stefan
@ 2021-01-18 14:03 ` Laurent Vivier
2021-01-18 20:18 ` Stefan
2021-02-13 19:07 ` Laurent Vivier
2021-02-13 19:22 ` Laurent Vivier
3 siblings, 1 reply; 7+ messages in thread
From: Laurent Vivier @ 2021-01-18 14:03 UTC (permalink / raw)
To: Stefan, qemu-devel, qemu-trivial; +Cc: qemu-stable
Le 09/01/2021 à 19:59, Stefan a écrit :
> The sizeof(struct ifreq) is 40 for 64 bit and 32 for 32 bit architectures.
> This structure contains a union of other structures, of which struct ifmap
> is the biggest for 64 bit architectures. Calling ioclt(…, SIOCGIFCONF, …)
> fills a struct sockaddr of that union, and do_ioctl_ifconf() only considered
> that struct sockaddr for the size of the union, which has the same size as
> struct ifmap on 32 bit architectures. So do_ioctl_ifconf() assumed a wrong
> size of 32 for struct ifreq instead of the correct size of 40 on 64 bit
> architectures.
>
> The fix makes do_ioctl_ifconf() handle struct ifmap as the biggest part of
> the union, treating struct ifreq with the correct size.
>
> Signed-off-by: Stefan <stefan-guix@vodafonemail.de>
> ---
> linux-user/syscall.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
> index d182890ff0..15a6abadc1 100644
> --- a/linux-user/syscall.c
> +++ b/linux-user/syscall.c
> @@ -4902,6 +4902,7 @@ static abi_long do_ioctl_ifconf(const IOCTLEntry *ie, uint8_t *buf_temp,
> struct ifconf *host_ifconf;
> uint32_t outbufsz;
> const argtype ifreq_arg_type[] = { MK_STRUCT(STRUCT_sockaddr_ifreq) };
> + const argtype ifreq_max_type[] = { MK_STRUCT(STRUCT_ifmap_ifreq) };
Why don't you simply replace STRUCT_sockaddr_ifreq by STRUCT_ifmap_ifreq rather than introducing a
new constant?
> int target_ifreq_size;
> int nb_ifreq;
> int free_buf = 0;
> @@ -4925,7 +4926,7 @@ static abi_long do_ioctl_ifconf(const IOCTLEntry *ie, uint8_t *buf_temp,
>
> host_ifconf = (struct ifconf *)(unsigned long)buf_temp;
> target_ifc_buf = (abi_long)(unsigned long)host_ifconf->ifc_buf;
> - target_ifreq_size = thunk_type_size(ifreq_arg_type, 0);
> + target_ifreq_size = thunk_type_size(ifreq_max_type, 0);
>
> if (target_ifc_buf != 0) {
> target_ifc_len = host_ifconf->ifc_len;
>
In the "if (!is_error(ret))", why don't you use the same size with the part that copies back the
values to the user?
Thanks,
Laurent
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH 1/1] linux-user/syscall: Fix do_ioctl_ifconf() for 64 bit targets.
2021-01-18 14:03 ` Laurent Vivier
@ 2021-01-18 20:18 ` Stefan
2021-02-06 14:11 ` Stefan
0 siblings, 1 reply; 7+ messages in thread
From: Stefan @ 2021-01-18 20:18 UTC (permalink / raw)
To: Laurent Vivier; +Cc: qemu-trivial, qemu-devel, qemu-stable
Hi Laurent!
Thanks for your response.
> Why don't you simply replace STRUCT_sockaddr_ifreq by STRUCT_ifmap_ifreq rather than introducing a
> new constant?
Because STRUCT_sockaddr_ifreq is the union part to be filled and is needed as an argument to thunk_convert() in this loop below:
for (i = 0; i < nb_ifreq ; i++) {
thunk_convert(argptr + i * target_ifreq_size,
host_ifc_buf + i * sizeof(struct ifreq),
ifreq_arg_type, THUNK_TARGET);
}
> In the "if (!is_error(ret))", why don't you use the same size with the part that copies back the
> values to the user?
I’m not sure if I understand your question correctly. Well, ioclt(…, SIOCGIFCONF, …) returns an array of struct ifreq, which contains a union, of which only struct sockaddr ifr_addr needs to be filled. But that union element is not the biggest element on 64 bit architectures.
Without the fix the returned data is not an array of struct ifreq but an array of some artificial struct:
struct sockaddr_ifreq {
char ifr_name[IFNAMSIZ]; /* Interface name */
struct sockaddr ifr_addr;
}
That artificial struct is too short for 64 bit architectures. On real x86_64 systems the size of the array returned by ioclt(…, SIOCGIFCONF, …) is a multiple of 40 bytes, the sizeof(struct ifreq). And it’s also a multiple of 40 on real aarch64 systems. However, on x86_64 emulating aarch64 with qemu, the returned array size is only a multiple of 32 bytes, which is wrong. It is enough to fill only 32 bytes with thunk_convert() and ifreq_arg_type is also the proper type, but the array element increase has to be 40 bytes.
I hope this clarifies your question.
Bye
Stefan
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/1] linux-user/syscall: Fix do_ioctl_ifconf() for 64 bit targets.
2021-01-09 18:59 [PATCH 1/1] linux-user/syscall: Fix do_ioctl_ifconf() for 64 bit targets Stefan
2021-01-17 13:00 ` Stefan
2021-01-18 14:03 ` Laurent Vivier
@ 2021-02-13 19:07 ` Laurent Vivier
2021-02-13 19:22 ` Laurent Vivier
3 siblings, 0 replies; 7+ messages in thread
From: Laurent Vivier @ 2021-02-13 19:07 UTC (permalink / raw)
To: Stefan, qemu-devel, qemu-trivial; +Cc: qemu-stable
Le 09/01/2021 à 19:59, Stefan a écrit :
> The sizeof(struct ifreq) is 40 for 64 bit and 32 for 32 bit architectures.
> This structure contains a union of other structures, of which struct ifmap
> is the biggest for 64 bit architectures. Calling ioclt(…, SIOCGIFCONF, …)
> fills a struct sockaddr of that union, and do_ioctl_ifconf() only considered
> that struct sockaddr for the size of the union, which has the same size as
> struct ifmap on 32 bit architectures. So do_ioctl_ifconf() assumed a wrong
> size of 32 for struct ifreq instead of the correct size of 40 on 64 bit
> architectures.
>
> The fix makes do_ioctl_ifconf() handle struct ifmap as the biggest part of
> the union, treating struct ifreq with the correct size.
>
> Signed-off-by: Stefan <stefan-guix@vodafonemail.de>
> ---
> linux-user/syscall.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
> index d182890ff0..15a6abadc1 100644
> --- a/linux-user/syscall.c
> +++ b/linux-user/syscall.c
> @@ -4902,6 +4902,7 @@ static abi_long do_ioctl_ifconf(const IOCTLEntry *ie, uint8_t *buf_temp,
> struct ifconf *host_ifconf;
> uint32_t outbufsz;
> const argtype ifreq_arg_type[] = { MK_STRUCT(STRUCT_sockaddr_ifreq) };
> + const argtype ifreq_max_type[] = { MK_STRUCT(STRUCT_ifmap_ifreq) };
> int target_ifreq_size;
> int nb_ifreq;
> int free_buf = 0;
> @@ -4925,7 +4926,7 @@ static abi_long do_ioctl_ifconf(const IOCTLEntry *ie, uint8_t *buf_temp,
>
> host_ifconf = (struct ifconf *)(unsigned long)buf_temp;
> target_ifc_buf = (abi_long)(unsigned long)host_ifconf->ifc_buf;
> - target_ifreq_size = thunk_type_size(ifreq_arg_type, 0);
> + target_ifreq_size = thunk_type_size(ifreq_max_type, 0);
>
> if (target_ifc_buf != 0) {
> target_ifc_len = host_ifconf->ifc_len;
>
Reviewed-by: Laurent Vivier <laurent@vivier.eu>
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH 1/1] linux-user/syscall: Fix do_ioctl_ifconf() for 64 bit targets.
2021-01-09 18:59 [PATCH 1/1] linux-user/syscall: Fix do_ioctl_ifconf() for 64 bit targets Stefan
` (2 preceding siblings ...)
2021-02-13 19:07 ` Laurent Vivier
@ 2021-02-13 19:22 ` Laurent Vivier
3 siblings, 0 replies; 7+ messages in thread
From: Laurent Vivier @ 2021-02-13 19:22 UTC (permalink / raw)
To: Stefan, qemu-devel, qemu-trivial; +Cc: qemu-stable
Le 09/01/2021 à 19:59, Stefan a écrit :
> The sizeof(struct ifreq) is 40 for 64 bit and 32 for 32 bit architectures.
> This structure contains a union of other structures, of which struct ifmap
> is the biggest for 64 bit architectures. Calling ioclt(…, SIOCGIFCONF, …)
> fills a struct sockaddr of that union, and do_ioctl_ifconf() only considered
> that struct sockaddr for the size of the union, which has the same size as
> struct ifmap on 32 bit architectures. So do_ioctl_ifconf() assumed a wrong
> size of 32 for struct ifreq instead of the correct size of 40 on 64 bit
> architectures.
>
> The fix makes do_ioctl_ifconf() handle struct ifmap as the biggest part of
> the union, treating struct ifreq with the correct size.
>
> Signed-off-by: Stefan <stefan-guix@vodafonemail.de>
> ---
> linux-user/syscall.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
> index d182890ff0..15a6abadc1 100644
> --- a/linux-user/syscall.c
> +++ b/linux-user/syscall.c
> @@ -4902,6 +4902,7 @@ static abi_long do_ioctl_ifconf(const IOCTLEntry *ie, uint8_t *buf_temp,
> struct ifconf *host_ifconf;
> uint32_t outbufsz;
> const argtype ifreq_arg_type[] = { MK_STRUCT(STRUCT_sockaddr_ifreq) };
> + const argtype ifreq_max_type[] = { MK_STRUCT(STRUCT_ifmap_ifreq) };
> int target_ifreq_size;
> int nb_ifreq;
> int free_buf = 0;
> @@ -4925,7 +4926,7 @@ static abi_long do_ioctl_ifconf(const IOCTLEntry *ie, uint8_t *buf_temp,
>
> host_ifconf = (struct ifconf *)(unsigned long)buf_temp;
> target_ifc_buf = (abi_long)(unsigned long)host_ifconf->ifc_buf;
> - target_ifreq_size = thunk_type_size(ifreq_arg_type, 0);
> + target_ifreq_size = thunk_type_size(ifreq_max_type, 0);
>
> if (target_ifc_buf != 0) {
> target_ifc_len = host_ifconf->ifc_len;
>
Applied to my linux-user-for-6.0 branch.
Thanks,
Laurent
^ permalink raw reply [flat|nested] 7+ messages in thread