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

qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed

* [Qemu-devel] [PATCH 0/2] linux-user: netlink cleanup
@ 2016-06-16 19:01 Laurent Vivier
  2016-06-16 19:01 ` [Qemu-devel] [PATCH 1/2] linux-user: fd_trans_host_to_target_data() must process only received data Laurent Vivier
  2016-06-16 19:01 ` [Qemu-devel] [PATCH 2/2] linux-user: don't swap NLMSG_DATA() fields Laurent Vivier
  0 siblings, 2 replies; 6+ messages in thread
From: Laurent Vivier @ 2016-06-16 19:01 UTC (permalink / raw)
  To: Riku Voipio; +Cc: qemu-devel, 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

^ permalink raw reply	[flat|nested] 6+ messages in thread

* [Qemu-devel] [PATCH 1/2] linux-user: fd_trans_host_to_target_data() must process only received data
  2016-06-16 19:01 [Qemu-devel] [PATCH 0/2] linux-user: netlink cleanup Laurent Vivier
@ 2016-06-16 19:01 ` Laurent Vivier
  2016-06-16 21:05   ` Peter Maydell
  2016-06-16 19:01 ` [Qemu-devel] [PATCH 2/2] linux-user: don't swap NLMSG_DATA() fields Laurent Vivier
  1 sibling, 1 reply; 6+ messages in thread
From: Laurent Vivier @ 2016-06-16 19:01 UTC (permalink / raw)
  To: Riku Voipio; +Cc: qemu-devel, 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

^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [Qemu-devel] [PATCH 1/2] linux-user: fd_trans_host_to_target_data() must process only received data
  2016-06-16 19:01 ` [Qemu-devel] [PATCH 1/2] linux-user: fd_trans_host_to_target_data() must process only received data Laurent Vivier
@ 2016-06-16 21:05   ` Peter Maydell
  0 siblings, 0 replies; 6+ messages in thread
From: Peter Maydell @ 2016-06-16 21:05 UTC (permalink / raw)
  To: Laurent Vivier; +Cc: Riku Voipio, QEMU Developers

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

^ permalink raw reply	[flat|nested] 6+ messages in thread

* [Qemu-devel] [PATCH 2/2] linux-user: don't swap NLMSG_DATA() fields
  2016-06-16 19:01 [Qemu-devel] [PATCH 0/2] linux-user: netlink cleanup Laurent Vivier
  2016-06-16 19:01 ` [Qemu-devel] [PATCH 1/2] linux-user: fd_trans_host_to_target_data() must process only received data Laurent Vivier
@ 2016-06-16 19:01 ` Laurent Vivier
  2016-06-16 21:09   ` Peter Maydell
  1 sibling, 1 reply; 6+ messages in thread
From: Laurent Vivier @ 2016-06-16 19:01 UTC (permalink / raw)
  To: Riku Voipio; +Cc: qemu-devel, 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

^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [Qemu-devel] [PATCH 2/2] linux-user: don't swap NLMSG_DATA() fields
  2016-06-16 19:01 ` [Qemu-devel] [PATCH 2/2] linux-user: don't swap NLMSG_DATA() fields Laurent Vivier
@ 2016-06-16 21:09   ` Peter Maydell
  2016-06-16 22:09     ` Laurent Vivier
  0 siblings, 1 reply; 6+ messages in thread
From: Peter Maydell @ 2016-06-16 21:09 UTC (permalink / raw)
  To: Laurent Vivier; +Cc: Riku Voipio, QEMU Developers

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

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [Qemu-devel] [PATCH 2/2] linux-user: don't swap NLMSG_DATA() fields
  2016-06-16 21:09   ` Peter Maydell
@ 2016-06-16 22:09     ` Laurent Vivier
  0 siblings, 0 replies; 6+ messages in thread
From: Laurent Vivier @ 2016-06-16 22:09 UTC (permalink / raw)
  To: Peter Maydell; +Cc: Riku Voipio, QEMU Developers



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

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2016-06-16 22:10 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2016-06-16 19:01 [Qemu-devel] [PATCH 0/2] linux-user: netlink cleanup Laurent Vivier
2016-06-16 19:01 ` [Qemu-devel] [PATCH 1/2] linux-user: fd_trans_host_to_target_data() must process only received data Laurent Vivier
2016-06-16 21:05   ` Peter Maydell
2016-06-16 19:01 ` [Qemu-devel] [PATCH 2/2] linux-user: don't swap NLMSG_DATA() fields Laurent Vivier
2016-06-16 21:09   ` Peter Maydell
2016-06-16 22:09     ` Laurent Vivier

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).