There are not many, and they are all simple mistakes that ended up
being committed. Remove them.
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Message-Id: <20181213223737.11793-2-pbonzini@redhat.com>
Reviewed-by: Wainer dos Santos Moschetta <wainersm@redhat.com>
Acked-by: Richard Henderson <richard.henderson@linaro.org>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Most list head structs need not be given a name. In most cases the
name is given just in case one is going to use QTAILQ_LAST, QTAILQ_PREV
or reverse iteration, but this does not apply to lists of other kinds,
and even for QTAILQ in practice this is only rarely needed. In addition,
we will soon reimplement those macros completely so that they do not
need a name for the head struct. So clean up everything, not giving a
name except in the rare case where it is necessary.
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Currently fork_exec() fork()s, and then creates and connects the
child socket which it uses for communication with the parent in
the child process. This is awkward because the child has no
mechanism to report failure back to the parent, which might end
up blocked forever in accept(). The child code also has an issue
pointed out by Coverity (CID 1005727), where if the qemu_socket()
call fails it will pass -1 as a file descriptor to connect().
Fix these issues by moving the creation of the child's end of
the socket to before the fork(), where we are in a position to
handle a possible failure.
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Samuel Thibault <samuel.thibault@ens-lyon.org>
Now that socreate() can never fail, we can remove the code
that was trying to handle that situation.
In particular this removes code in tcp_connect() that
provoked Coverity to complain (CID 1005724): in
closesocket(accept(inso->s, (struct sockaddr *)&addr, &addrlen));
if the accept() call fails then we pass closesocket() -1
instead of a valid file descriptor.
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Samuel Thibault <samuel.thibault@ens-lyon.org>
The slirp socreate() function can only fail if the attempt
to malloc() the struct socket fails. Switch to using
g_new() instead, which will allow us to remove the
error-handling code from its callers.
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Samuel Thibault <samuel.thibault@ens-lyon.org>
Coverity complains (CID 1005726) that we might pass -1 as the fd
argument to send() in slirp_send(), because we previously checked for
"so->s == -1 && so->extra". The case of "so->s == -1 but so->extra
NULL" should not in theory happen, but it is hard to guarantee
because various places in the code do so->s = qemu_socket(...) and so
will end up with so->s == -1 on failure, and not all the paths which
call that always throw away the socket in that case (eg
tcp_fconnect()). So just check specifically for the condition and
fail slirp_send().
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Samuel Thibault <samuel.thibault@ens-lyon.org>
This new usernet option can be used to add data for option 66 (tftp
server name) in the BOOTP reply, which is useful in PXE based automatic
OS install such as OpenBSD.
Signed-off-by: Fam Zheng <famz@redhat.com>
Reviewed-by: Thomas Huth <thuth@redhat.com>
Tested-by: Gerd Hoffmann <kraxel@redhat.com>
Signed-off-by: Samuel Thibault <samuel.thibault@ens-lyon.org>
When user provides a long domainname or hostname that doesn't fit in the
DHCP packet, we mustn't overflow the response packet buffer. Instead,
report errors, following the g_warning() in the slirp->vdnssearch
branch.
Also check the strlen against 256 when initializing slirp, which limit
is also from the protocol where one byte represents the string length.
This gives an early error before the warning which is harder to notice
or diagnose.
Reported-by: Thomas Huth <thuth@redhat.com>
Reviewed-by: Thomas Huth <thuth@redhat.com>
Cc: qemu-stable@nongnu.org
Signed-off-by: Fam Zheng <famz@redhat.com>
Tested-by: Gerd Hoffmann <kraxel@redhat.com>
Signed-off-by: Samuel Thibault <samuel.thibault@ens-lyon.org>
Adds EXTERNAL attribute definition to qemu timers subsystem and assigns
it to virtual clock timers, used in slirp (ICMP IPv6) and ui (key queue).
Virtual clock processing in rr mode can use this attribute instead of a
separate clock type.
Fixes: 87f4fe7653
Fixes: 775a412bf8
Fixes: 9888091404
Signed-off-by: Artem Pisarenko <artem.k.pisarenko@gmail.com>
Message-Id: <e771f96ab94e86b54b9a783c974f2af3009fe5d1.1539764043.git.artem.k.pisarenko@gmail.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
That patch series introduced new virtual clock type for use in external
subsystems. It breaks desired behavior in non-record/replay usage
scenarios due to a small change to existing behavior. Processing of
virtual timers belonging to new clock type is kicked off to the main
loop, which makes these timers asynchronous with vCPU thread and,
in icount mode, with whole guest execution. This breaks expected
determinism in non-record/replay icount mode of emulation where these
"external subsystems" are isolated from the host (i.e. they are
external only to guest core, not to the entire emulation environment).
Example for slirp ("user" backend for network device):
User runs qemu in icount mode with rtc clock=vm without any external
communication interfaces but with "-netdev user,restrict=on". It expects
deterministic execution, because network services are emulated inside
qemu and isolated from host. There are no reasons to get reply from DHCP
server with different delay or something like that.
The next patches revert reimplements the same changes in a better way.
This reverts commit 87f4fe7653.
This reverts commit 775a412bf8.
This reverts commit 9888091404.
Signed-off-by: Artem Pisarenko <artem.k.pisarenko@gmail.com>
Message-Id: <18b1e7c8f155fe26976f91be06bde98eef6f8751.1539764043.git.artem.k.pisarenko@gmail.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Commit 27d92ebc5e handled the case where the TCP
connection is abruptly closed via a RST packet, by checking for the ECONNRESET
errno. However it does not consider the case where the connection has been
half-closed by the host (FIN/ACK), then the host socket is disconnected. For
example, if the host application calls close() on the socket, then the
application exits.
In this case, the socket still exists due to the file descriptor in SLIRP, but
it is disconnected. recv() does not indicate an error since an orderly socket
close has previously occurred. The socket will then be stuck in FIN_WAIT_2,
until the peer sends FIN/ACK or a timeout occurs. Instead we can send a RST
to the peer and transition to the CLOSED state.
Signed-off-by: Gavin Grant <gavingrant@protonmail.com>
Signed-off-by: Samuel Thibault <samuel.thibault@ens-lyon.org>
On Linux, SOCK_DGRAM+IPPROTO_ICMP sockets give only the ICMP packet when
read from. On macOS, however, the socket acts like a SOCK_RAW socket
and includes the IP header as well.
This change strips the extra IP header from the received packet on macOS
before sending it to the guest. SOCK_DGRAM ICMP sockets aren't
supported on other BSDs, but we enable this behavior for them as well to
treat the sockets the same as raw sockets.
Signed-off-by: Andrew Oates <aoates@google.com>
Signed-off-by: Samuel Thibault <samuel.thibault@ens-lyon.org>
and fix confusing datasize name into gapsize in m_inc.
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Samuel Thibault <samuel.thibault@ens-lyon.org>
ICMP implementation for IPv6 uses timers based on virtual clock.
This is incorrect because this service is not related to the guest state,
and its events should not be recorded and replayed.
This patch changes using virtual clock to the new virtual_ext clock.
Signed-off-by: Pavel Dovgalyuk <Pavel.Dovgaluk@ispras.ru>
Message-Id: <20180912082007.3228.91491.stgit@pasha-VirtualBox>
Reviewed-by: Samuel Thibault <samuel.thibault@ens-lyon.org>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
The data in an mbuf buffer is not necessarily at the start of the
allocated buffer. (For instance m_adj() allows data to be trimmed
from the start by just advancing the pointer and reducing the length.)
This means that the allocated buffer size (m->m_size) and the
amount of space from the m_data pointer to the end of the
buffer (M_ROOM(m)) are not necessarily the same.
Commit 864036e251 tried to change the m_inc() function from
taking the new allocated-buffer-size to taking the new room-size,
but forgot to change the initial "do we already have enough space"
check. This meant that if we were trying to extend a buffer which
had a leading gap between the buffer start and the data, we might
incorrectly decide it didn't need to be extended, and then
overrun the end of the buffer, causing memory corruption and
an eventual crash.
Change the "already big enough?" condition from checking the
argument against m->m_size to checking against M_ROOM().
This only makes a difference for the callsite in m_cat();
the other three callsites all start with a freshly allocated
mbuf from m_get(), which will have m->m_size == M_ROOM(m).
Fixes: 864036e251
Fixes: https://bugs.launchpad.net/qemu/+bug/1785670
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
Reviewed-by: Samuel Thibault <samuel.thibault@ens-lyon.org>
Message-id: 20180807114501.12370-1-peter.maydell@linaro.org
Tested-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
Coding style changes to the m_inc routine and minor refactoring.
Reported-by: ZDI Disclosures <zdi-disclosures@trendmicro.com>
Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org>
Signed-off-by: Samuel Thibault <samuel.thibault@ens-lyon.org>
While reassembling incoming fragmented datagrams, 'm_cat' routine
extends the 'mbuf' buffer, if it has insufficient room. It computes
a wrong buffer size, which leads to overwriting adjacent heap buffer
area. Correct this size computation in m_cat.
Reported-by: ZDI Disclosures <zdi-disclosures@trendmicro.com>
Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org>
Signed-off-by: Samuel Thibault <samuel.thibault@ens-lyon.org>
Move check to where it actually is useful, and reduce scope of 'len'
variable along the way.
Signed-off-by: Samuel Thibault <samuel.thibault@ens-lyon.org>
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
The checksum field of a NC-SI packet contains a value that may be
included in each command and response. The verification is optional
but the Linux driver does so when a non-zero value is provided. Let's
extend the model to compute the checksum value and exercise a little
more the Linux driver.
See section "8.2.2.3 - 2's Complement Checksum Compensation" in the
Network Controller Sideband Interface (NC-SI) Specification for more
details.
Signed-off-by: Cédric Le Goater <clg@kaod.org>
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Tested-by: Joel Stanley <joel@jms.id.au>
Signed-off-by: Samuel Thibault <samuel.thibault@ens-lyon.org>
Command 0x17 'Get Parameters' is used to get configuration parameter
values currently in effect on the controller and it is mandatory in
the NS-CI specification.
Provide a minimum response to exercise the kernel.
Signed-off-by: Cédric Le Goater <clg@kaod.org>
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Tested-by: Joel Stanley <joel@jms.id.au>
Signed-off-by: Samuel Thibault <samuel.thibault@ens-lyon.org>
Signed-off-by: Cédric Le Goater <clg@kaod.org>
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Tested-by: Joel Stanley <joel@jms.id.au>
Signed-off-by: Samuel Thibault <samuel.thibault@ens-lyon.org>
If the receive window presented to the guest closes, slirp should send a
window update once the window reopens sufficiently, rather than forcing
the guest to send a window probe, which can take several seconds.
Signed-off-by: James Clarke <jrtc27@jrtc27.com>
Signed-off-by: Samuel Thibault <samuel.thibault@ens-lyon.org>
This follows 3929766fb3e4 ('slirp: disable Nagle in outgoing connections'):
for the same reasons, ingoing connections should have the Nagle algorithm disabled.
Signed-off-by: Samuel Thibault <samuel.thibault@ens-lyon.org>
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
When setting up an outgoing user mode networking TCP connection,
disable the Nagle algorithm in the host-side connection. Either the
guest is already doing Nagle, in which case there is no point in doing
it twice, or it has chosen to disable it, in which case we should
respect that choice.
This change speeds up GDB remote debugging over TCP over user mode
networking (with GDB runing on the guest) by multiple orders of
magnitude, and has been part of the local patches applied by pkgsrc
since 2012 with no reported ill effects.
Signed-off-by: Andreas Gustafsson <gson@gson.org>
Reviewed-by: Kamil Rytarowski <n54@gmx.com>
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Signed-off-by: Samuel Thibault <samuel.thibault@ens-lyon.org>
This patch will allow the user to include the domainname option in
replies from the built-in DHCP server.
Signed-off-by: Benjamin Drung <benjamin.drung@profitbricks.com>
Signed-off-by: Samuel Thibault <samuel.thibault@ens-lyon.org>
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Reviewed-by: Thomas Huth <thuth@redhat.com>
Signed-off-by: Samuel Thibault <samuel.thibault@ens-lyon.org>
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Tested-by: Thomas Huth <thuth@redhat.com>
Signed-off-by: Samuel Thibault <samuel.thibault@ens-lyon.org>
Host: Mac OS 10.12.5
Compiler: Apple LLVM version 8.1.0 (clang-802.0.42)
slirp/ip6_icmp.c:80:38: warning: taking address of packed member 'ip_src' of class or
structure 'ip6' may result in an unaligned pointer value
[-Waddress-of-packed-member]
IN6_IS_ADDR_UNSPECIFIED(&ip->ip_src)) {
^~~~~~~~~~
/usr/include/netinet6/in6.h:238:42: note: expanded from macro 'IN6_IS_ADDR_UNSPECIFIED'
((*(const __uint32_t *)(const void *)(&(a)->s6_addr[0]) == 0) && \
^
Reported-by: John Arbuckle <programmingkidx@gmail.com>
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Signed-off-by: Samuel Thibault <samuel.thibault@ens-lyon.org>
98c63057d2 ('slirp: Factorizing
tcpiphdr structure with an union') introduced a memset call to clear
possibly-undefined fields in ti. This however overwrites src/dst/pr which
are used below.
So let us clear only the unused fields.
This should fix some rare cases (some RST cases, keep alive probes)
where packets would be sent to 0.0.0.0.
Signed-off-by: Tao Wu <lepton@google.com>
Signed-off-by: Samuel Thibault <samuel.thibault@ens-lyon.org>
Modify the pre_save method on VMStateDescription to return an int
rather than void so that it potentially can fail.
Changed zillions of devices to make them return 0; the only
case I've made it return non-0 is hw/intc/s390_flic_kvm.c that already
had an error_report/return case.
Note: If you add an error exit in your pre_save you must emit
an error_report to say why.
Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
Message-Id: <20170925112917.21340-2-dgilbert@redhat.com>
Reviewed-by: Peter Xu <peterx@redhat.com>
Reviewed-by: Cornelia Huck <cohuck@redhat.com>
Reviewed-by: Juan Quintela <quintela@redhat.com>
Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
NULL sockets are used for NDP, BOOTP, and other critical operations.
If the topmost mbuf in a NULL session is blocked pending resolution,
it may cause problems if it blocks other packets with a NULL socket.
So do not add mbufs with a NULL socket field to the same session.
Signed-off-by: Kevin Cernekee <cernekee@chromium.org>
Signed-off-by: Samuel Thibault <samuel.thibault@ens-lyon.org>
if_output() originally sent one mbuf per call and used the slirp->next_m
variable to keep track of where it left off. But nowadays it tries to
send all of the mbufs from the fastq, and one mbuf from each session on
the batchq. The next_m variable is both redundant and harmful: there is
a case[0] involving delayed packets in which next_m ends up pointing
to &slirp->if_batchq when an active session still exists, and this
blocks all traffic for that session until qemu is restarted.
The test case was created to reproduce a problem that was seen on
long-running Chromium OS VM tests[1] which rapidly create and
destroy ssh connections through hostfwd.
[0] https://pastebin.com/NNy6LreF
[1] https://bugs.chromium.org/p/chromium/issues/detail?id=766323
Signed-off-by: Kevin Cernekee <cernekee@chromium.org>
Signed-off-by: Samuel Thibault <samuel.thibault@ens-lyon.org>
The if_fastq and if_batchq contain not only packets, but queues of packets
for the same socket. When sofree frees a socket, it thus has to clear ifq_so
from all the packets from the queues, not only the first.
Signed-off-by: Samuel Thibault <samuel.thibault@ens-lyon.org>
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Cc: qemu-stable@nongnu.org
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
While parsing dhcp options string in 'dhcp_decode', if an options'
length 'len' appeared towards the end of 'bp_vend' array, ensuing
read could lead to an OOB memory access issue. Add check to avoid it.
This is CVE-2017-11434.
Reported-by: Reno Robert <renorobert@gmail.com>
Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org>
Signed-off-by: Samuel Thibault <samuel.thibault@ens-lyon.org>
sosendoob() can return a failure code, but all its callers ignore it.
This is OK in sbappend(), as the comment there states -- we will try
again later in sowrite(). Add a (void) cast to tell Coverity so.
In sowrite() we do need to check the return value -- we should handle
a write failure in sosendoob() the same way we handle a write failure
for the normal data.
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
Signed-off-by: Samuel Thibault <samuel.thibault@ens-lyon.org>
The code in sosendoob() assumes that slirp_send() always
succeeds, but it might return an OS error code (for instance
if the other end has disconnected). Catch these and return
the caller either -1 on error or the number of urgent bytes
actually written. (None of the callers check this return
value currently, though.)
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
Signed-off-by: Samuel Thibault <samuel.thibault@ens-lyon.org>
In a fork_exec() error path we try to closesocket(s) when s might
be a negative number because the thing that failed was the
qemu_socket() call. Add a guard so we don't do this.
(Spotted by Coverity: CID 1005727 issue 1 of 2.)
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
Signed-off-by: Samuel Thibault <samuel.thibault@ens-lyon.org>
I used the clang-tidy qemu-round check to generate the fix:
https://github.com/elmarco/clang-tools-extra
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Signed-off-by: Samuel Thibault <samuel.thibault@ens-lyon.org>
They are indpendent, and nowadays almost every device register things
with qdev->vmsd.
Signed-off-by: Juan Quintela <quintela@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Reviewed-by: Peter Xu <peterx@redhat.com>
We can replace the four remaining calls of register_savevm() by
calls to register_savevm_live(). So we can remove the function and
as we don't allocate anymore the ops pointer with g_new0()
we don't have to free it then.
Signed-off-by: Laurent Vivier <lvivier@redhat.com>
Reviewed-by: Juan Quintela <quintela@redhat.com>
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
Move all the frontend struct and methods to a seperate unit. This avoids
accidentally mixing backend and frontend calls, and helps with readabilty.
Make qemu_chr_replay() a macro shared by both char and char-fe.
Export qemu_chr_write(), and use a macro for qemu_chr_write_all()
(nb: yes, CharBackend is for char frontend :)
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
So they are all in one place. The following patch will move serial &
parallel declarations to the respective headers.
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
When forwarding TCP packets, the internal tcpiphdr struct length was wrongly
used inside the IP header. This commit changes the behaviour to what is used
by tcp_output.c, using the correct full IP header + payload length.
Signed-off-by: Sjors Gielen <sjors@sjorsgielen.nl>
Signed-off-by: Samuel Thibault <samuel.thibault@ens-lyon.org>
Spotted by ASAN:
/x86_64/hmp/pc-0.12:
=================================================================
==22538==ERROR: LeakSanitizer: detected memory leaks
Direct leak of 224 byte(s) in 1 object(s) allocated from:
#0 0x7f0f63cdee60 in malloc (/lib64/libasan.so.3+0xc6e60)
#1 0x556f11ff32d7 in tcp_newtcpcb /home/elmarco/src/qemu/slirp/tcp_subr.c:250
#2 0x556f11fdb1d1 in tcp_listen /home/elmarco/src/qemu/slirp/socket.c:688
#3 0x556f11fca9d5 in slirp_add_hostfwd /home/elmarco/src/qemu/slirp/slirp.c:1052
#4 0x556f11f8db41 in slirp_hostfwd /home/elmarco/src/qemu/net/slirp.c:506
#5 0x556f11f8dd83 in hmp_hostfwd_add /home/elmarco/src/qemu/net/slirp.c:535
There might be a better way to fix this, but calling slirp tcp_close()
doesn't work.
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Signed-off-by: Samuel Thibault <samuel.thibault@ens-lyon.org>
This bug was introduced by https://github.com/qemu/qemu/commit/98c6305
Signed-off-by: Tao Wu <lepton@google.com>
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Signed-off-bu: Samuel Thibault <samuel.thibault@ens-lyon.org>
This converts the remaining components, except for the top level
loop, to VMState.
Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
Reviewed-by: Juan Quintela <quintela@redhat.com>
Signed-off-by: Samuel Thibault <samuel.thibault@ens-lyon.org>
Paolo Bonzini