Re: [Qemu-devel] [PATCH] linux-user: Fix crashes in ioctl(SIOCGIFCONF) when ifc_buf is NULL.

2018-10-13 Thread Laurent Vivier 
On 12/10/2018 21:02, Laurent Vivier wrote:
> On 09/10/2018 09:45, Kan Li wrote:
>> Summary:
>> This is to fix bug https://bugs.launchpad.net/qemu/+bug/1796754.
>> It is valid for ifc_buf to be NULL according to
>> http://man7.org/linux/man-pages/man7/netdevice.7.html.
>>
>> Signed-off-by: Kan Li 
>> ---
>>  linux-user/syscall.c | 56 
>>  1 file changed, 31 insertions(+), 25 deletions(-)
>>
>> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
>> index ae3c0dfef7..fbab98d4f7 100644
>> --- a/linux-user/syscall.c
>> +++ b/linux-user/syscall.c
>> @@ -4134,28 +4134,33 @@ static abi_long do_ioctl_ifconf(const IOCTLEntry 
>> *ie, uint8_t *buf_temp,
>>  unlock_user(argptr, arg, 0);
>>  
>>  host_ifconf = (struct ifconf *)(unsigned long)buf_temp;
>> -target_ifc_len = host_ifconf->ifc_len;
>>  target_ifc_buf = (abi_long)(unsigned long)host_ifconf->ifc_buf;
>>  
>> -target_ifreq_size = thunk_type_size(ifreq_arg_type, 0);
>> -nb_ifreq = target_ifc_len / target_ifreq_size;
>> -host_ifc_len = nb_ifreq * sizeof(struct ifreq);
>> +if (target_ifc_buf != 0) {
>> +target_ifc_len = host_ifconf->ifc_len;
>>  
>> -outbufsz = sizeof(*host_ifconf) + host_ifc_len;
>> -if (outbufsz > MAX_STRUCT_SIZE) {
>> -/* We can't fit all the extents into the fixed size buffer.
>> - * Allocate one that is large enough and use it instead.
>> - */
>> -host_ifconf = malloc(outbufsz);
>> -if (!host_ifconf) {
>> -return -TARGET_ENOMEM;
>> +target_ifreq_size = thunk_type_size(ifreq_arg_type, 0);

In fact, the target_ifreq_size is used later even if target_ifc_buf is
NULL, so you have to move it out of the "if" body.

Thanks,
Laurent

Re: [Qemu-devel] [PATCH] linux-user: Fix crashes in ioctl(SIOCGIFCONF) when ifc_buf is NULL.

2018-10-12 Thread Laurent Vivier 
On 09/10/2018 09:45, Kan Li wrote:
> Summary:
> This is to fix bug https://bugs.launchpad.net/qemu/+bug/1796754.
> It is valid for ifc_buf to be NULL according to
> http://man7.org/linux/man-pages/man7/netdevice.7.html.
> 
> Signed-off-by: Kan Li 
> ---
>  linux-user/syscall.c | 56 
>  1 file changed, 31 insertions(+), 25 deletions(-)
> 
> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
> index ae3c0dfef7..fbab98d4f7 100644
> --- a/linux-user/syscall.c
> +++ b/linux-user/syscall.c
> @@ -4134,28 +4134,33 @@ static abi_long do_ioctl_ifconf(const IOCTLEntry *ie, 
> uint8_t *buf_temp,
>  unlock_user(argptr, arg, 0);
>  
>  host_ifconf = (struct ifconf *)(unsigned long)buf_temp;
> -target_ifc_len = host_ifconf->ifc_len;
>  target_ifc_buf = (abi_long)(unsigned long)host_ifconf->ifc_buf;
>  
> -target_ifreq_size = thunk_type_size(ifreq_arg_type, 0);
> -nb_ifreq = target_ifc_len / target_ifreq_size;
> -host_ifc_len = nb_ifreq * sizeof(struct ifreq);
> +if (target_ifc_buf != 0) {
> +target_ifc_len = host_ifconf->ifc_len;
>  
> -outbufsz = sizeof(*host_ifconf) + host_ifc_len;
> -if (outbufsz > MAX_STRUCT_SIZE) {
> -/* We can't fit all the extents into the fixed size buffer.
> - * Allocate one that is large enough and use it instead.
> - */
> -host_ifconf = malloc(outbufsz);
> -if (!host_ifconf) {
> -return -TARGET_ENOMEM;
> +target_ifreq_size = thunk_type_size(ifreq_arg_type, 0);
> +nb_ifreq = target_ifc_len / target_ifreq_size;
> +host_ifc_len = nb_ifreq * sizeof(struct ifreq);
> +
> +outbufsz = sizeof(*host_ifconf) + host_ifc_len;
> +if (outbufsz > MAX_STRUCT_SIZE) {
> +/* We can't fit all the extents into the fixed size buffer.
> + * Allocate one that is large enough and use it instead.
> + */
> +host_ifconf = malloc(outbufsz);
> +if (!host_ifconf) {
> +return -TARGET_ENOMEM;
> +}
> +memcpy(host_ifconf, buf_temp, sizeof(*host_ifconf));
> +free_buf = 1;
>  }
> -memcpy(host_ifconf, buf_temp, sizeof(*host_ifconf));
> -free_buf = 1;
> -}
> -host_ifc_buf = (char*)host_ifconf + sizeof(*host_ifconf);
> +host_ifc_buf = (char*)host_ifconf + sizeof(*host_ifconf);
>  
> -host_ifconf->ifc_len = host_ifc_len;
> +host_ifconf->ifc_len = host_ifc_len;
> +} else {
> +  host_ifc_buf = NULL;
> +}
>  host_ifconf->ifc_buf = host_ifc_buf;
>  
>  ret = get_errno(safe_ioctl(fd, ie->host_cmd, host_ifconf));
> @@ -4178,15 +4183,16 @@ static abi_long do_ioctl_ifconf(const IOCTLEntry *ie, 
> uint8_t *buf_temp,
>  thunk_convert(argptr, host_ifconf, arg_type, THUNK_TARGET);
>  unlock_user(argptr, arg, target_size);
>  
> - /* copy ifreq[] to target user */
> -
> -argptr = lock_user(VERIFY_WRITE, target_ifc_buf, target_ifc_len, 0);
> -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);
> +if (target_ifc_buf != 0) {
> +/* copy ifreq[] to target user */
> +argptr = lock_user(VERIFY_WRITE, target_ifc_buf, target_ifc_len, 
> 0);
> +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);
> +}
> +unlock_user(argptr, target_ifc_buf, target_ifc_len);
>  }
> -unlock_user(argptr, target_ifc_buf, target_ifc_len);
>  }
>  
>  if (free_buf) {
> 

Applied to my branch linux-user-for-3.1

Thanks,
Laurent

Re: [Qemu-devel] [PATCH] linux-user: Fix crashes in ioctl(SIOCGIFCONF) when ifc_buf is NULL.

2018-10-12 Thread Laurent Vivier 
On 09/10/2018 09:45, Kan Li wrote:
> Summary:
> This is to fix bug https://bugs.launchpad.net/qemu/+bug/1796754.
> It is valid for ifc_buf to be NULL according to
> http://man7.org/linux/man-pages/man7/netdevice.7.html.
> 
> Signed-off-by: Kan Li 
> ---
>  linux-user/syscall.c | 56 
>  1 file changed, 31 insertions(+), 25 deletions(-)

Reviewed-by: Laurent Vivier

[Qemu-devel] [PATCH] linux-user: Fix crashes in ioctl(SIOCGIFCONF) when ifc_buf is NULL.

2018-10-09 Thread Kan Li 


Summary:
This is to fix bug https://bugs.launchpad.net/qemu/+bug/1796754.
It is valid for ifc_buf to be NULL according to
http://man7.org/linux/man-pages/man7/netdevice.7.html.

Signed-off-by: Kan Li 
---
 linux-user/syscall.c | 56 
 1 file changed, 31 insertions(+), 25 deletions(-)

diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index ae3c0dfef7..fbab98d4f7 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -4134,28 +4134,33 @@ static abi_long do_ioctl_ifconf(const IOCTLEntry *ie, 
uint8_t *buf_temp,
 unlock_user(argptr, arg, 0);
 
 host_ifconf = (struct ifconf *)(unsigned long)buf_temp;
-target_ifc_len = host_ifconf->ifc_len;
 target_ifc_buf = (abi_long)(unsigned long)host_ifconf->ifc_buf;
 
-target_ifreq_size = thunk_type_size(ifreq_arg_type, 0);
-nb_ifreq = target_ifc_len / target_ifreq_size;
-host_ifc_len = nb_ifreq * sizeof(struct ifreq);
+if (target_ifc_buf != 0) {
+target_ifc_len = host_ifconf->ifc_len;
 
-outbufsz = sizeof(*host_ifconf) + host_ifc_len;
-if (outbufsz > MAX_STRUCT_SIZE) {
-/* We can't fit all the extents into the fixed size buffer.
- * Allocate one that is large enough and use it instead.
- */
-host_ifconf = malloc(outbufsz);
-if (!host_ifconf) {
-return -TARGET_ENOMEM;
+target_ifreq_size = thunk_type_size(ifreq_arg_type, 0);
+nb_ifreq = target_ifc_len / target_ifreq_size;
+host_ifc_len = nb_ifreq * sizeof(struct ifreq);
+
+outbufsz = sizeof(*host_ifconf) + host_ifc_len;
+if (outbufsz > MAX_STRUCT_SIZE) {
+/* We can't fit all the extents into the fixed size buffer.
+ * Allocate one that is large enough and use it instead.
+ */
+host_ifconf = malloc(outbufsz);
+if (!host_ifconf) {
+return -TARGET_ENOMEM;
+}
+memcpy(host_ifconf, buf_temp, sizeof(*host_ifconf));
+free_buf = 1;
 }
-memcpy(host_ifconf, buf_temp, sizeof(*host_ifconf));
-free_buf = 1;
-}
-host_ifc_buf = (char*)host_ifconf + sizeof(*host_ifconf);
+host_ifc_buf = (char*)host_ifconf + sizeof(*host_ifconf);
 
-host_ifconf->ifc_len = host_ifc_len;
+host_ifconf->ifc_len = host_ifc_len;
+} else {
+  host_ifc_buf = NULL;
+}
 host_ifconf->ifc_buf = host_ifc_buf;
 
 ret = get_errno(safe_ioctl(fd, ie->host_cmd, host_ifconf));
@@ -4178,15 +4183,16 @@ static abi_long do_ioctl_ifconf(const IOCTLEntry *ie, 
uint8_t *buf_temp,
 thunk_convert(argptr, host_ifconf, arg_type, THUNK_TARGET);
 unlock_user(argptr, arg, target_size);
 
-   /* copy ifreq[] to target user */
-
-argptr = lock_user(VERIFY_WRITE, target_ifc_buf, target_ifc_len, 0);
-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);
+if (target_ifc_buf != 0) {
+/* copy ifreq[] to target user */
+argptr = lock_user(VERIFY_WRITE, target_ifc_buf, target_ifc_len, 
0);
+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);
+}
+unlock_user(argptr, target_ifc_buf, target_ifc_len);
 }
-unlock_user(argptr, target_ifc_buf, target_ifc_len);
 }
 
 if (free_buf) {
-- 
2.17.1