Re: [Qemu-devel] A use-after-free in slirp
Hi Samuel,
On 08/24/2017 08:42 PM, Samuel Thibault wrote:
Hello,
Thanks for the reproducer you sent me offline. Here is a fix which makes
a lot of sense and fixes the reproducer. Could you try it with your
whole testcase?
Could somebody also review the patch?
Your patch looks correct.
It seems worth to add a soqfree() function for readability:
static inline void soqfree(struct quehead *ifq)
{
struct mbuf *ifm;
for (ifm = ifq->ifs_next; ifm != ifq; ifm = ifm->ifs_next) {
ifm->ifq_so = NULL;
}
ifq->ifq_so = NULL;
}
then use:
if (ifq->ifq_so == so) {
soqfree(ifq);
}
same with slirp->if_batchq
Also can you send your patch in the proper format?
Thanks!
Regards,
Phil.
Samuel
commit 1a3a763509fad895c907e6978ea034a5c19ee370
Author: Samuel Thibault
Date: Fri Aug 25 01:35:53 2017 +0200
slirp: fix clearing ifq_so from pending packets
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
Acked-by: Philippe Mathieu-Daudé
diff --git a/slirp/socket.c b/slirp/socket.c
index ecec0295a9..4203b80542 100644
--- a/slirp/socket.c
+++ b/slirp/socket.c
@@ -66,21 +66,29 @@ void
sofree(struct socket *so)
{
Slirp *slirp = so->slirp;
- struct mbuf *ifm;
-
- for (ifm = (struct mbuf *) slirp->if_fastq.qh_link;
- (struct quehead *) ifm != &slirp->if_fastq;
- ifm = ifm->ifq_next) {
-if (ifm->ifq_so == so) {
- ifm->ifq_so = NULL;
+ struct mbuf *ifq;
+
+ for (ifq = (struct mbuf *) slirp->if_fastq.qh_link;
+ (struct quehead *) ifq != &slirp->if_fastq;
+ ifq = ifq->ifq_next) {
+if (ifq->ifq_so == so) {
+ struct mbuf *ifm;
+ ifq->ifq_so = NULL;
+ for (ifm = ifq->ifs_next; ifm != ifq; ifm = ifm->ifs_next) {
+ifm->ifq_so = NULL;
+ }
}
}
- for (ifm = (struct mbuf *) slirp->if_batchq.qh_link;
- (struct quehead *) ifm != &slirp->if_batchq;
- ifm = ifm->ifq_next) {
-if (ifm->ifq_so == so) {
- ifm->ifq_so = NULL;
+ for (ifq = (struct mbuf *) slirp->if_batchq.qh_link;
+ (struct quehead *) ifq != &slirp->if_batchq;
+ ifq = ifq->ifq_next) {
+if (ifq->ifq_so == so) {
+ struct mbuf *ifm;
+ ifq->ifq_so = NULL;
+ for (ifm = ifq->ifs_next; ifm != ifq; ifm = ifm->ifs_next) {
+ifm->ifq_so = NULL;
+ }
}
}
Re: [Qemu-devel] A use-after-free in slirp
Hello,
Thanks for the reproducer you sent me offline. Here is a fix which makes
a lot of sense and fixes the reproducer. Could you try it with your
whole testcase?
Could somebody also review the patch?
Samuel
commit 1a3a763509fad895c907e6978ea034a5c19ee370
Author: Samuel Thibault
Date: Fri Aug 25 01:35:53 2017 +0200
slirp: fix clearing ifq_so from pending packets
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
diff --git a/slirp/socket.c b/slirp/socket.c
index ecec0295a9..4203b80542 100644
--- a/slirp/socket.c
+++ b/slirp/socket.c
@@ -66,21 +66,29 @@ void
sofree(struct socket *so)
{
Slirp *slirp = so->slirp;
- struct mbuf *ifm;
-
- for (ifm = (struct mbuf *) slirp->if_fastq.qh_link;
- (struct quehead *) ifm != &slirp->if_fastq;
- ifm = ifm->ifq_next) {
-if (ifm->ifq_so == so) {
- ifm->ifq_so = NULL;
+ struct mbuf *ifq;
+
+ for (ifq = (struct mbuf *) slirp->if_fastq.qh_link;
+ (struct quehead *) ifq != &slirp->if_fastq;
+ ifq = ifq->ifq_next) {
+if (ifq->ifq_so == so) {
+ struct mbuf *ifm;
+ ifq->ifq_so = NULL;
+ for (ifm = ifq->ifs_next; ifm != ifq; ifm = ifm->ifs_next) {
+ifm->ifq_so = NULL;
+ }
}
}
- for (ifm = (struct mbuf *) slirp->if_batchq.qh_link;
- (struct quehead *) ifm != &slirp->if_batchq;
- ifm = ifm->ifq_next) {
-if (ifm->ifq_so == so) {
- ifm->ifq_so = NULL;
+ for (ifq = (struct mbuf *) slirp->if_batchq.qh_link;
+ (struct quehead *) ifq != &slirp->if_batchq;
+ ifq = ifq->ifq_next) {
+if (ifq->ifq_so == so) {
+ struct mbuf *ifm;
+ ifq->ifq_so = NULL;
+ for (ifm = ifq->ifs_next; ifm != ifq; ifm = ifm->ifs_next) {
+ifm->ifq_so = NULL;
+ }
}
}
Re: [Qemu-devel] A use-after-free in slirp
Hello Samuel, +-- On Wed, 23 Aug 2017, Samuel Thibault wrote --+ | The paste is not available any more. Is it really very large? It's usually | really better to just send it by mail, so it's archived in the mailing list | etc. Yes, stack-trace was quite long. === ==2704==ERROR: AddressSanitizer: heap-use-after-free on address 0x6141018c at pc 0x003921ea145d bp 0x7fd49c4fc940 sp 0x7fd49c4fc930 READ of size 4 at 0x6141018c thread T2 #0 0x3921ea145c in if_start slirp/if.c:230 #1 0x3921ea1015 in if_output slirp/if.c:141 #2 0x3921eadf1f in ip_output slirp/ip_output.c:85 #3 0x3921ed229e in tcp_respond slirp/tcp_subr.c:218 #4 0x3921ecc959 in tcp_input slirp/tcp_input.c:1392 #5 0x3921eab799 in ip_input slirp/ip_input.c:206 #6 0x3921eb6529 in slirp_input slirp/slirp.c:872 #7 0x3921e7c56f in net_slirp_receive net/slirp.c:119 #8 0x3921e60fe0 in nc_sendv_compat net/net.c:707 #9 0x3921e61170 in qemu_deliver_packet_iov net/net.c:734 #10 0x3921e67c53 in qemu_net_queue_deliver_iov net/queue.c:179 #11 0x3921e67e5b in qemu_net_queue_send_iov net/queue.c:224 #12 0x3921e61395 in qemu_sendv_packet_async net/net.c:770 #13 0x3921e613c2 in qemu_sendv_packet net/net.c:778 #14 0x3921e6961e in net_hub_receive_iov net/hub.c:72 #15 0x3921e69c12 in net_hub_port_receive_iov net/hub.c:123 #16 0x3921e61155 in qemu_deliver_packet_iov net/net.c:732 #17 0x3921e67ae7 in qemu_net_queue_deliver net/queue.c:164 #18 0x3921e67d59 in qemu_net_queue_send net/queue.c:199 #19 0x3921e60d58 in qemu_send_packet_async_with_flags net/net.c:661 #20 0x3921e60d90 in qemu_send_packet_async net/net.c:668 #21 0x3921e60dbd in qemu_send_packet net/net.c:674 #22 0x3921bef076 in ne2000_ioport_write hw/net/ne2000.c:302 #23 0x3921bf07c4 in ne2000_write hw/net/ne2000.c:688 #24 0x3921668a95 in memory_region_write_accessor /home/test/qemu/memory.c:529 #25 0x3921668d6e in access_with_adjusted_size /home/test/qemu/memory.c:595 #26 0x392166f4ca in memory_region_dispatch_write /home/test/qemu/memory.c:1337 #27 0x39215c633c in address_space_write_continue /home/test/qemu/exec.c:2942 #28 0x39215c65df in address_space_write /home/test/qemu/exec.c:2987 #29 0x39215c6df3 in address_space_rw /home/test/qemu/exec.c:3089 #30 0x39216a3159 in kvm_handle_io /home/test/qemu/accel/kvm/kvm-all.c:1795 #31 0x39216a4425 in kvm_cpu_exec /home/test/qemu/accel/kvm/kvm-all.c:2035 #32 0x3921636a6c in qemu_kvm_cpu_thread_fn /home/test/qemu/cpus.c:1128 #33 0x7fd4a5f4336c in start_thread (/lib64/libpthread.so.0+0x736c) #34 0x7fd4a5c7bbbe in __GI___clone (/lib64/libc.so.6+0x110bbe) 0x6141018c is located 332 bytes inside of 416-byte region [0x61410040,0x614101e0) freed by thread T2 here: #0 0x7fd4a967c4b8 in __interceptor_free (/lib64/libasan.so.4+0xde4b8) #1 0x3921ebf027 in sofree slirp/socket.c:106 #2 0x3921ed2cd5 in tcp_close slirp/tcp_subr.c:334 #3 0x3921eca600 in tcp_input slirp/tcp_input.c:948 #4 0x3921eab799 in ip_input slirp/ip_input.c:206 #5 0x3921eb6529 in slirp_input slirp/slirp.c:872 #6 0x3921e7c56f in net_slirp_receive net/slirp.c:119 #7 0x3921e60fe0 in nc_sendv_compat net/net.c:707 #8 0x3921e61170 in qemu_deliver_packet_iov net/net.c:734 #9 0x3921e67c53 in qemu_net_queue_deliver_iov net/queue.c:179 #10 0x3921e67e5b in qemu_net_queue_send_iov net/queue.c:224 #11 0x3921e61395 in qemu_sendv_packet_async net/net.c:770 #12 0x3921e613c2 in qemu_sendv_packet net/net.c:778 #13 0x3921e6961e in net_hub_receive_iov net/hub.c:72 #14 0x3921e69c12 in net_hub_port_receive_iov net/hub.c:123 #15 0x3921e61155 in qemu_deliver_packet_iov net/net.c:732 #16 0x3921e67ae7 in qemu_net_queue_deliver net/queue.c:164 #17 0x3921e67d59 in qemu_net_queue_send net/queue.c:199 #18 0x3921e60d58 in qemu_send_packet_async_with_flags net/net.c:661 #19 0x3921e60d90 in qemu_send_packet_async net/net.c:668 #20 0x3921e60dbd in qemu_send_packet net/net.c:674 #21 0x3921bef076 in ne2000_ioport_write hw/net/ne2000.c:302 #22 0x3921bf07c4 in ne2000_write hw/net/ne2000.c:688 #23 0x3921668a95 in memory_region_write_accessor /home/test/qemu/memory.c:529 #24 0x3921668d6e in access_with_adjusted_size /home/test/qemu/memory.c:595 #25 0x392166f4ca in memory_region_dispatch_write /home/test/qemu/memory.c:1337 #26 0x39215c633c in address_space_write_continue /home/test/qemu/exec.c:2942 #27 0x39215c65df in address_space_write /home/test/qemu/exec.c:2987 #28 0x39215c6df3 in address_space_rw /home/test/qemu/exec.c:3089 #29 0x39216a3159 in kvm_handle_io /home/test/qemu/accel/kvm/kvm-all.c:1795 previously allocated by thread T2 here: #0 0x7fd4a967c850 in malloc (/lib64/libasan.so.4+0xde850) #1 0x3921ebeaa5 in socreate slirp/socket.c:51 #2 0x3921ec7184 in tcp_input slirp/tcp_input.c:432 #3 0x3921eab799 in ip_input slirp/ip_input.c:206 #4 0x
Re: [Qemu-devel] A use-after-free in slirp
Hello, P J P, on jeu. 03 août 2017 17:45:06 +0530, wrote: > ==31922==ERROR: AddressSanitizer: heap-use-after-free on address > 0x6141ff8c at pc 0x56485de28ea0 bp 0x7f00f44fc950 sp 0x7f00f44fc940 > READ of size 4 at 0x6141ff8c thread T2 > #0 0x56485de28e9f in if_start slirp/if.c:230 > #1 0x56485de28a58 in if_output slirp/if.c:141 > #2 0x56485de35173 in ip_output slirp/ip_output.c:85 > #3 0x56485de57c48 in tcp_respond slirp/tcp_subr.c:218 > #4 0x56485de52440 in tcp_input slirp/tcp_input.c:1392 > #5 0x56485de329ef in ip_input slirp/ip_input.c:206 > #6 0x56485de3cf93 in slirp_input slirp/slirp.c:872 > #7 0x56485de0726d in net_slirp_receive net/slirp.c:119 > #8 0x56485ddee24d in nc_sendv_compat net/net.c:707 > #9 0x56485ddee3dd in qemu_deliver_packet_iov net/net.c:734 > #10 0x56485ddf422c in qemu_net_queue_deliver_iov net/queue.c:179 > ... Please don't strip the output :) The most interesting part is what exactly freed this. > A full trace output can be seen > > here -> https://paste.fedoraproject.org/paste/gh~hDctqUQ8uVt6UdG~zbg The paste is not available any more. Is it really very large? It's usually really better to just send it by mail, so it's archived in the mailing list etc. Samuel
