diff mbox series

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

Message ID 71ad91e4ee9f41f439086d8f9de60501ad304859.1597129029.git.scw@google.com (mailing list archive)
State New, archived
Headers show
Series fcntl, sockopt, and ioctl options | expand

Commit Message

Shu-Chun Weng Aug. 11, 2020, 7:09 a.m. UTC 
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(-)

Comments

Laurent Vivier Aug. 11, 2020, 2:21 p.m. UTC | #1 
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
Shu-Chun Weng Aug. 11, 2020, 8:04 p.m. UTC | #2 
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 <(564)%20586-2798>..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
>
diff mbox series

Patch

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;