[Qemu-devel] [PATCH 0/2] linux-user: netlink cleanup

[Qemu-devel] [PATCH 0/2] linux-user: netlink cleanup

[Laurent Vivier]

Some cleanup to avoid memory corruption while netlink
helpers are processing data stream.

Laurent Vivier (2):
  linux-user: fd_trans_host_to_target_data() must process only received
    data
  linux-user: don't swap NLMSG_DATA() fields

 linux-user/syscall.c | 74 ++++++++++++++++++++++++++++++----------------------
 1 file changed, 43 insertions(+), 31 deletions(-)

-- 
2.5.5

[Qemu-devel] [PATCH 1/2] linux-user: fd_trans_host_to_target_data() must process only received data

[Laurent Vivier]

if we process the whole buffer, the netlink helpers can try
to swap invalid data.

Signed-off-by: Laurent Vivier <laurent@vivier.eu>
---
 linux-user/syscall.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index 0b937ca..3c30437 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -2987,7 +2987,7 @@ static abi_long do_sendrecvmsg_locked(int fd, struct target_msghdr *msgp,
             len = ret;
             if (fd_trans_host_to_target_data(fd)) {
                 ret = fd_trans_host_to_target_data(fd)(msg.msg_iov->iov_base,
-                                                       msg.msg_iov->iov_len);
+                                                       len);
             } else {
                 ret = host_to_target_cmsg(msgp, &msg);
             }
-- 
2.5.5

[Qemu-devel] [PATCH 2/2] linux-user: don't swap NLMSG_DATA() fields

[Laurent Vivier]

If the structure pointed by NLMSG_DATA() is bigger
than the size of NLMSG_DATA(), don't swap its fields
to avoid memory corruption.

Signed-off-by: Laurent Vivier <laurent@vivier.eu>
---
 linux-user/syscall.c | 72 ++++++++++++++++++++++++++++++----------------------
 1 file changed, 42 insertions(+), 30 deletions(-)

diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index 3c30437..a0a3e65 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -1944,29 +1944,35 @@ static abi_long host_to_target_data_route(struct nlmsghdr *nlh)
     case RTM_NEWLINK:
     case RTM_DELLINK:
     case RTM_GETLINK:
-        ifi = NLMSG_DATA(nlh);
-        ifi->ifi_type = tswap16(ifi->ifi_type);
-        ifi->ifi_index = tswap32(ifi->ifi_index);
-        ifi->ifi_flags = tswap32(ifi->ifi_flags);
-        ifi->ifi_change = tswap32(ifi->ifi_change);
-        host_to_target_link_rtattr(IFLA_RTA(ifi),
-                                   nlmsg_len - NLMSG_LENGTH(sizeof(*ifi)));
+        if (nlh->nlmsg_len >= NLMSG_LENGTH(sizeof(*ifi))) {
+            ifi = NLMSG_DATA(nlh);
+            ifi->ifi_type = tswap16(ifi->ifi_type);
+            ifi->ifi_index = tswap32(ifi->ifi_index);
+            ifi->ifi_flags = tswap32(ifi->ifi_flags);
+            ifi->ifi_change = tswap32(ifi->ifi_change);
+            host_to_target_link_rtattr(IFLA_RTA(ifi),
+                                       nlmsg_len - NLMSG_LENGTH(sizeof(*ifi)));
+        }
         break;
     case RTM_NEWADDR:
     case RTM_DELADDR:
     case RTM_GETADDR:
-        ifa = NLMSG_DATA(nlh);
-        ifa->ifa_index = tswap32(ifa->ifa_index);
-        host_to_target_addr_rtattr(IFA_RTA(ifa),
-                                   nlmsg_len - NLMSG_LENGTH(sizeof(*ifa)));
+        if (nlh->nlmsg_len >= NLMSG_LENGTH(sizeof(*ifa))) {
+            ifa = NLMSG_DATA(nlh);
+            ifa->ifa_index = tswap32(ifa->ifa_index);
+            host_to_target_addr_rtattr(IFA_RTA(ifa),
+                                       nlmsg_len - NLMSG_LENGTH(sizeof(*ifa)));
+        }
         break;
     case RTM_NEWROUTE:
     case RTM_DELROUTE:
     case RTM_GETROUTE:
-        rtm = NLMSG_DATA(nlh);
-        rtm->rtm_flags = tswap32(rtm->rtm_flags);
-        host_to_target_route_rtattr(RTM_RTA(rtm),
-                                    nlmsg_len - NLMSG_LENGTH(sizeof(*rtm)));
+        if (nlh->nlmsg_len >= NLMSG_LENGTH(sizeof(*rtm))) {
+            rtm = NLMSG_DATA(nlh);
+            rtm->rtm_flags = tswap32(rtm->rtm_flags);
+            host_to_target_route_rtattr(RTM_RTA(rtm),
+                                        nlmsg_len - NLMSG_LENGTH(sizeof(*rtm)));
+        }
         break;
     default:
         return -TARGET_EINVAL;
@@ -2082,30 +2088,36 @@ static abi_long target_to_host_data_route(struct nlmsghdr *nlh)
         break;
     case RTM_NEWLINK:
     case RTM_DELLINK:
-        ifi = NLMSG_DATA(nlh);
-        ifi->ifi_type = tswap16(ifi->ifi_type);
-        ifi->ifi_index = tswap32(ifi->ifi_index);
-        ifi->ifi_flags = tswap32(ifi->ifi_flags);
-        ifi->ifi_change = tswap32(ifi->ifi_change);
-        target_to_host_link_rtattr(IFLA_RTA(ifi), nlh->nlmsg_len -
-                                   NLMSG_LENGTH(sizeof(*ifi)));
+        if (nlh->nlmsg_len >= NLMSG_LENGTH(sizeof(*ifi))) {
+            ifi = NLMSG_DATA(nlh);
+            ifi->ifi_type = tswap16(ifi->ifi_type);
+            ifi->ifi_index = tswap32(ifi->ifi_index);
+            ifi->ifi_flags = tswap32(ifi->ifi_flags);
+            ifi->ifi_change = tswap32(ifi->ifi_change);
+            target_to_host_link_rtattr(IFLA_RTA(ifi), nlh->nlmsg_len -
+                                       NLMSG_LENGTH(sizeof(*ifi)));
+        }
         break;
     case RTM_GETADDR:
     case RTM_NEWADDR:
     case RTM_DELADDR:
-        ifa = NLMSG_DATA(nlh);
-        ifa->ifa_index = tswap32(ifa->ifa_index);
-        target_to_host_addr_rtattr(IFA_RTA(ifa), nlh->nlmsg_len -
-                                   NLMSG_LENGTH(sizeof(*ifa)));
+        if (nlh->nlmsg_len >= NLMSG_LENGTH(sizeof(*ifa))) {
+            ifa = NLMSG_DATA(nlh);
+            ifa->ifa_index = tswap32(ifa->ifa_index);
+            target_to_host_addr_rtattr(IFA_RTA(ifa), nlh->nlmsg_len -
+                                       NLMSG_LENGTH(sizeof(*ifa)));
+        }
         break;
     case RTM_GETROUTE:
         break;
     case RTM_NEWROUTE:
     case RTM_DELROUTE:
-        rtm = NLMSG_DATA(nlh);
-        rtm->rtm_flags = tswap32(rtm->rtm_flags);
-        target_to_host_route_rtattr(RTM_RTA(rtm), nlh->nlmsg_len -
-                                    NLMSG_LENGTH(sizeof(*rtm)));
+        if (nlh->nlmsg_len >= NLMSG_LENGTH(sizeof(*rtm))) {
+            rtm = NLMSG_DATA(nlh);
+            rtm->rtm_flags = tswap32(rtm->rtm_flags);
+            target_to_host_route_rtattr(RTM_RTA(rtm), nlh->nlmsg_len -
+                                        NLMSG_LENGTH(sizeof(*rtm)));
+        }
         break;
     default:
         return -TARGET_EOPNOTSUPP;
-- 
2.5.5

Re: [Qemu-devel] [PATCH 1/2] linux-user: fd_trans_host_to_target_data() must process only received data

[Peter Maydell]

On 16 June 2016 at 20:01, Laurent Vivier <laurent@vivier.eu> wrote:
> if we process the whole buffer, the netlink helpers can try
> to swap invalid data.
>
> Signed-off-by: Laurent Vivier <laurent@vivier.eu>
> ---
>  linux-user/syscall.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
> index 0b937ca..3c30437 100644
> --- a/linux-user/syscall.c
> +++ b/linux-user/syscall.c
> @@ -2987,7 +2987,7 @@ static abi_long do_sendrecvmsg_locked(int fd, struct target_msghdr *msgp,
>              len = ret;
>              if (fd_trans_host_to_target_data(fd)) {
>                  ret = fd_trans_host_to_target_data(fd)(msg.msg_iov->iov_base,
> -                                                       msg.msg_iov->iov_len);
> +                                                       len);
>              } else {
>                  ret = host_to_target_cmsg(msgp, &msg);
>              }

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

Re: [Qemu-devel] [PATCH 2/2] linux-user: don't swap NLMSG_DATA() fields

[Peter Maydell]

On 16 June 2016 at 20:01, Laurent Vivier <laurent@vivier.eu> wrote:
> If the structure pointed by NLMSG_DATA() is bigger
> than the size of NLMSG_DATA(), don't swap its fields
> to avoid memory corruption.
>
> Signed-off-by: Laurent Vivier <laurent@vivier.eu>
> ---

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

Can this actually happen in normal operation?

thanks
-- PMM

Re: [Qemu-devel] [PATCH 2/2] linux-user: don't swap NLMSG_DATA() fields

[Laurent Vivier]

Le 16/06/2016 à 23:09, Peter Maydell a écrit :
> On 16 June 2016 at 20:01, Laurent Vivier <laurent@vivier.eu> wrote:
>> If the structure pointed by NLMSG_DATA() is bigger
>> than the size of NLMSG_DATA(), don't swap its fields
>> to avoid memory corruption.
>>
>> Signed-off-by: Laurent Vivier <laurent@vivier.eu>
>> ---
> 
> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
> 
> Can this actually happen in normal operation?

Yes, I've detected that debugging "apt-get update" on debian jessie with
qemu-s390x. This is the first call to netlink:

00 00 00 14     nlmsg_len=20
00 16           nlmsg_type=RTM_GETADDR
03 01           nlmsg_flags=0x0301
57 62 b7 fb     nlmsg_seq=0x5762b7fb
00 00 00 00     nlmsg_pid=0
00 00 00 00	NLMSG_DATA() = struct ifaddrmsg

struct ifaddrmsg {
        __u8            ifa_family;
        __u8            ifa_prefixlen;
        __u8            ifa_flags;
        __u8            ifa_scope;
        __u32           ifa_index;
};

Laurent