[PATCH 0/4] small linux-user/syscall:do_setsockopt() cleanups and fixes

[PATCH 0/4] small linux-user/syscall:do_setsockopt() cleanups and fixes

[Michael Tokarev]

A few cleanups and fixes for linux-user/syscall:do_setsockopt().

Michael Tokarev (4):
  linux-user/syscall: do_setsockopt: fix SOL_ALG.ALG_SET_KEY
  linux-user/syscall: do_setsockopt(): make ip_mreq local to the place
    it is used and inline target_to_host_ip_mreq()
  linux-user/syscall: do_setsockopt(): make ip_mreq_source local to the
    place where it is used
  linux-user/syscall: do_setsockopt(): eliminate goto in switch for SO_SNDTIMEO

 linux-user/syscall.c | 64 +++++++++++++++++---------------------------
 1 file changed, 25 insertions(+), 39 deletions(-)

-- 
2.39.2

[PATCH v2 1/4] linux-user/syscall: do_setsockopt: fix SOL_ALG.ALG_SET_KEY

[Michael Tokarev]

This setsockopt accepts zero-lengh optlen (current qemu implementation
does not allow this).  Also, there's no need to make a copy of the key,
it is enough to use lock_user() (which accepts zero length already).

Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2197
Fixes: f31dddd2fc "linux-user: Add support for setsockopt() option SOL_ALG"
Signed-off-by: Michael Tokarev <mjt@tls.msk.ru>
---
 linux-user/syscall.c | 9 ++-------
 1 file changed, 2 insertions(+), 7 deletions(-)

v2: fix commit message wording

diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index e12d969c2e..5c7728cfd4 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -2277,18 +2277,13 @@ static abi_long do_setsockopt(int sockfd, int level, int optname,
         switch (optname) {
         case ALG_SET_KEY:
         {
-            char *alg_key = g_malloc(optlen);
-
+            char *alg_key = lock_user(VERIFY_READ, optval_addr, optlen, 1);
             if (!alg_key) {
-                return -TARGET_ENOMEM;
-            }
-            if (copy_from_user(alg_key, optval_addr, optlen)) {
-                g_free(alg_key);
                 return -TARGET_EFAULT;
             }
             ret = get_errno(setsockopt(sockfd, level, optname,
                                        alg_key, optlen));
-            g_free(alg_key);
+            unlock_user(alg_key, optval_addr, optlen);
             break;
         }
         case ALG_SET_AEAD_AUTHSIZE:
-- 
2.39.2

[PATCH 2/4] linux-user/syscall: do_setsockopt(): make ip_mreq local to the place it is used and inline target_to_host_ip_mreq()

[Michael Tokarev]

ip_mreq is declared at the beginning of do_setsockopt(), while
it is used in only one place.  Move its declaration to that very
place and replace pointer to alloca()-allocated memory with the
structure itself.

target_to_host_ip_mreq() is used only once, inline it.

This change also properly handles TARGET_EFAULT when the address
is wrong.

Note: optlen here refers to target struct size, while host
struct size can potentially be different.

Signed-off-by: Michael Tokarev <mjt@tls.msk.ru>
---
 linux-user/syscall.c | 38 +++++++++++++++-----------------------
 1 file changed, 15 insertions(+), 23 deletions(-)

diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index 5c7728cfd4..733a2b91e9 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -1615,24 +1615,6 @@ static abi_long do_pipe(CPUArchState *cpu_env, abi_ulong pipedes,
     return get_errno(ret);
 }
 
-static inline abi_long target_to_host_ip_mreq(struct ip_mreqn *mreqn,
-                                              abi_ulong target_addr,
-                                              socklen_t len)
-{
-    struct target_ip_mreqn *target_smreqn;
-
-    target_smreqn = lock_user(VERIFY_READ, target_addr, len, 1);
-    if (!target_smreqn)
-        return -TARGET_EFAULT;
-    mreqn->imr_multiaddr.s_addr = target_smreqn->imr_multiaddr.s_addr;
-    mreqn->imr_address.s_addr = target_smreqn->imr_address.s_addr;
-    if (len == sizeof(struct target_ip_mreqn))
-        mreqn->imr_ifindex = tswapal(target_smreqn->imr_ifindex);
-    unlock_user(target_smreqn, target_addr, 0);
-
-    return 0;
-}
-
 static inline abi_long target_to_host_sockaddr(int fd, struct sockaddr *addr,
                                                abi_ulong target_addr,
                                                socklen_t len)
@@ -2067,7 +2049,6 @@ static abi_long do_setsockopt(int sockfd, int level, int optname,
 {
     abi_long ret;
     int val;
-    struct ip_mreqn *ip_mreq;
     struct ip_mreq_source *ip_mreq_source;
 
     switch(level) {
@@ -2111,15 +2092,26 @@ static abi_long do_setsockopt(int sockfd, int level, int optname,
             break;
         case IP_ADD_MEMBERSHIP:
         case IP_DROP_MEMBERSHIP:
+        {
+            struct ip_mreqn ip_mreq;
+            struct target_ip_mreqn *target_smreqn;
+
             if (optlen < sizeof (struct target_ip_mreq) ||
                 optlen > sizeof (struct target_ip_mreqn))
                 return -TARGET_EINVAL;
 
-            ip_mreq = (struct ip_mreqn *) alloca(optlen);
-            target_to_host_ip_mreq(ip_mreq, optval_addr, optlen);
-            ret = get_errno(setsockopt(sockfd, level, optname, ip_mreq, optlen));
-            break;
+            target_smreqn = lock_user(VERIFY_READ, optval_addr, optlen, 1);
+            if (!target_smreqn)
+                return -TARGET_EFAULT;
+            ip_mreq.imr_multiaddr.s_addr = target_smreqn->imr_multiaddr.s_addr;
+            ip_mreq.imr_address.s_addr = target_smreqn->imr_address.s_addr;
+            if (optlen == sizeof(struct target_ip_mreqn))
+                ip_mreq.imr_ifindex = tswapal(target_smreqn->imr_ifindex);
+            unlock_user(target_smreqn, optval_addr, 0);
 
+            ret = get_errno(setsockopt(sockfd, level, optname, &ip_mreq, optlen));
+            break;
+        }
         case IP_BLOCK_SOURCE:
         case IP_UNBLOCK_SOURCE:
         case IP_ADD_SOURCE_MEMBERSHIP:
-- 
2.39.2

[PATCH 3/4] linux-user/syscall: do_setsockopt(): make ip_mreq_source local to the place where it is used

[Michael Tokarev]

Signed-off-by: Michael Tokarev <mjt@tls.msk.ru>
---
 linux-user/syscall.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index 733a2b91e9..988b6422fb 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -2049,7 +2049,6 @@ static abi_long do_setsockopt(int sockfd, int level, int optname,
 {
     abi_long ret;
     int val;
-    struct ip_mreq_source *ip_mreq_source;
 
     switch(level) {
     case SOL_TCP:
@@ -2116,6 +2115,9 @@ static abi_long do_setsockopt(int sockfd, int level, int optname,
         case IP_UNBLOCK_SOURCE:
         case IP_ADD_SOURCE_MEMBERSHIP:
         case IP_DROP_SOURCE_MEMBERSHIP:
+        {
+            struct ip_mreq_source *ip_mreq_source;
+
             if (optlen != sizeof (struct target_ip_mreq_source))
                 return -TARGET_EINVAL;
 
@@ -2126,7 +2128,7 @@ static abi_long do_setsockopt(int sockfd, int level, int optname,
             ret = get_errno(setsockopt(sockfd, level, optname, ip_mreq_source, optlen));
             unlock_user (ip_mreq_source, optval_addr, 0);
             break;
-
+        }
         default:
             goto unimplemented;
         }
-- 
2.39.2

[PATCH 4/4] linux-user/syscall: do_setsockopt(): eliminate goto in switch for SO_SNDTIMEO

[Michael Tokarev]

There's identical code for SO_SNDTIMEO and SO_RCVTIMEO, currently
implemented using an ugly goto into another switch case.  Eliminate
that using arithmetic if, making code flow more natural.

Signed-off-by: Michael Tokarev <mjt@tls.msk.ru>
---
 linux-user/syscall.c | 11 ++++-------
 1 file changed, 4 insertions(+), 7 deletions(-)

diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index 988b6422fb..cdf26c43ba 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -2294,12 +2294,10 @@ static abi_long do_setsockopt(int sockfd, int level, int optname,
     case TARGET_SOL_SOCKET:
         switch (optname) {
         case TARGET_SO_RCVTIMEO:
+        case TARGET_SO_SNDTIMEO:
         {
                 struct timeval tv;
 
-                optname = SO_RCVTIMEO;
-
-set_timeout:
                 if (optlen != sizeof(struct target_timeval)) {
                     return -TARGET_EINVAL;
                 }
@@ -2308,13 +2306,12 @@ set_timeout:
                     return -TARGET_EFAULT;
                 }
 
-                ret = get_errno(setsockopt(sockfd, SOL_SOCKET, optname,
+                ret = get_errno(setsockopt(sockfd, SOL_SOCKET,
+                                optname == TARGET_SO_RCVTIMEO ?
+                                    SO_RCVTIMEO : SO_SNDTIMEO,
                                 &tv, sizeof(tv)));
                 return ret;
         }
-        case TARGET_SO_SNDTIMEO:
-                optname = SO_SNDTIMEO;
-                goto set_timeout;
         case TARGET_SO_ATTACH_FILTER:
         {
                 struct target_sock_fprog *tfprog;
-- 
2.39.2

Re: [PATCH 4/4] linux-user/syscall: do_setsockopt(): eliminate goto in switch for SO_SNDTIMEO

[Philippe Mathieu-Daudé]

On 31/3/24 12:07, Michael Tokarev wrote:
> There's identical code for SO_SNDTIMEO and SO_RCVTIMEO, currently
> implemented using an ugly goto into another switch case.  Eliminate
> that using arithmetic if, making code flow more natural.
> 
> Signed-off-by: Michael Tokarev <mjt@tls.msk.ru>
> ---
>   linux-user/syscall.c | 11 ++++-------
>   1 file changed, 4 insertions(+), 7 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH 0/4] small linux-user/syscall:do_setsockopt() cleanups and fixes

[Richard Henderson]

On 3/31/24 00:07, Michael Tokarev wrote:
> A few cleanups and fixes for linux-user/syscall:do_setsockopt().
> 
> Michael Tokarev (4):
>    linux-user/syscall: do_setsockopt: fix SOL_ALG.ALG_SET_KEY
>    linux-user/syscall: do_setsockopt(): make ip_mreq local to the place
>      it is used and inline target_to_host_ip_mreq()
>    linux-user/syscall: do_setsockopt(): make ip_mreq_source local to the
>      place where it is used
>    linux-user/syscall: do_setsockopt(): eliminate goto in switch for SO_SNDTIMEO
> 
>   linux-user/syscall.c | 64 +++++++++++++++++---------------------------
>   1 file changed, 25 insertions(+), 39 deletions(-)
> 

Queued, thanks.


r~