Re: [PATCH v2 2/8] linux-user: add missing UDP get/setsockopt option

[Shu-Chun Weng]

It does look like something that can be improved. The lines have been there for 14 years though: https://github.com/qemu/qemu/commit/53a5960aadd542dd27b8705ac30df154557d5ffc

The potential bug is triggered when the user passes in a 2-byte integer in getsockopt(), which seems uncommon -- do we have guest architectures that use 16-bit int type?

Shu-Chun

On Tue, Aug 11, 2020 at 7:21 AM Laurent Vivier <laurent@vivier.eu> wrote:

Le 11/08/2020 à 09:09, Shu-Chun Weng a écrit :
> SOL_UDP manipulate options at UDP level. All six options currently defined
> in linux source include/uapi/linux/udp.h take integer values.
>
> Signed-off-by: Shu-Chun Weng <scw@google.com>
> Reviewed-by: Laurent Vivier <laurent@vivier.eu>
> ---
> v1 -> v2:
>   Split out SOL_UDP into own patch.
>   Updated do_print_sockopt().
>
>  linux-user/strace.c  | 6 ++++++
>  linux-user/syscall.c | 7 +++++--
>  2 files changed, 11 insertions(+), 2 deletions(-)
>
> diff --git a/linux-user/strace.c b/linux-user/strace.c
> index 4fff24b880..854b54a2ad 100644
> --- a/linux-user/strace.c
> +++ b/linux-user/strace.c
> @@ -7,6 +7,7 @@
>  #include <sys/mount.h>
>  #include <arpa/inet.h>
>  #include <netinet/tcp.h>
> +#include <netinet/udp.h>
>  #include <linux/if_packet.h>
>  #include <linux/netlink.h>
>  #include <sched.h>
> @@ -2190,6 +2191,11 @@ static void do_print_sockopt(const char *name, abi_long arg1)
>          print_raw_param(TARGET_ABI_FMT_ld, optname, 0);
>          print_pointer(optval, 0);
>          break;
> +    case SOL_UDP:
> +        qemu_log("SOL_UDP,");
> +        print_raw_param(TARGET_ABI_FMT_ld, optname, 0);
> +        print_pointer(optval, 0);
> +        break;
>      case SOL_IP:
>          qemu_log("SOL_IP,");
>          print_raw_param(TARGET_ABI_FMT_ld, optname, 0);
> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
> index 5645862798..177eec5201 100644
> --- a/linux-user/syscall.c
> +++ b/linux-user/syscall.c
> @@ -53,6 +53,7 @@
>  //#include <sys/user.h>
>  #include <netinet/ip.h>
>  #include <netinet/tcp.h>
> +#include <netinet/udp.h>
>  #include <linux/wireless.h>
>  #include <linux/icmp.h>
>  #include <linux/icmpv6.h>
> @@ -1938,7 +1939,8 @@ static abi_long do_setsockopt(int sockfd, int level, int optname,
> 
>      switch(level) {
>      case SOL_TCP:
> -        /* TCP options all take an 'int' value.  */
> +    case SOL_UDP:
> +        /* TCP and UDP options all take an 'int' value.  */
>          if (optlen < sizeof(uint32_t))
>              return -TARGET_EINVAL;
> 
> @@ -2586,7 +2588,8 @@ get_timeout:
>          }
>          break;
>      case SOL_TCP:
> -        /* TCP options all take an 'int' value.  */
> +    case SOL_UDP:
> +        /* TCP and UDP options all take an 'int' value.  */
>      int_case:
>          if (get_user_u32(len, optlen))
>              return -TARGET_EFAULT;
>

Reviewed-by: Laurent Vivier <laurent@vivier.eu>

I'm wondering if the int_case of getsockopt() manages correctly the
length: length can be between 0 and sizeof(int), but the int_case only
uses a put_user_u32() or a put_user_u8(). Do we need the put_user_u16()?

Thanks,
Laurent

smime.p7s
Description: S/MIME Cryptographic Signature