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>
Acked-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
slirp/socket.c | 39 +++++++++++++++++++++++----------------
1 file changed, 23 insertions(+), 16 deletions(-)
diff --git a/slirp/socket.c b/slirp/socket.c
index ecec0295a9..cb7b5b608d 100644
--- a/slirp/socket.c
+++ b/slirp/socket.c
@@ -59,6 +59,27 @@ socreate(Slirp *slirp)
return(so);
}
+/*
+ * Remove references to so from the given message queue.
+ */
+static void
+soqfree(struct socket *so, struct quehead *qh)
+{
+ struct mbuf *ifq;
+
+ for (ifq = (struct mbuf *) qh->qh_link;
+ (struct quehead *) ifq != qh;
+ 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;
+ }
+ }
+ }
+}
+
/*
* remque and free a socket, clobber cache
*/
@@ -66,23 +87,9 @@ 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;
- }
- }
-
- 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;
- }
- }
+ soqfree(so, &slirp->if_fastq);
+ soqfree(so, &slirp->if_batchq);
if (so->so_emu==EMU_RSH && so->extra) {
sofree(so->extra);
--
2.14.1
Hi Sam, thanks for this patch :) On 08/25/2017 07:37 PM, Samuel Thibault wrote: > 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> > Acked-by: Philippe Mathieu-Daudé <f4bug@amsat.org> so let's raise this to: Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org> > --- > slirp/socket.c | 39 +++++++++++++++++++++++---------------- > 1 file changed, 23 insertions(+), 16 deletions(-) > > diff --git a/slirp/socket.c b/slirp/socket.c > index ecec0295a9..cb7b5b608d 100644 > --- a/slirp/socket.c > +++ b/slirp/socket.c > @@ -59,6 +59,27 @@ socreate(Slirp *slirp) > return(so); > } > > +/* > + * Remove references to so from the given message queue. > + */ > +static void > +soqfree(struct socket *so, struct quehead *qh) > +{ > + struct mbuf *ifq; > + > + for (ifq = (struct mbuf *) qh->qh_link; > + (struct quehead *) ifq != qh; > + 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; > + } > + } > + } > +} > + > /* > * remque and free a socket, clobber cache > */ > @@ -66,23 +87,9 @@ 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; > - } > - } > - > - 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; > - } > - } > + soqfree(so, &slirp->if_fastq); > + soqfree(so, &slirp->if_batchq); > > if (so->so_emu==EMU_RSH && so->extra) { > sofree(so->extra); >
Hello, So Wjjzhang and PJP, can you confirm that this fixes your uses? Samuel Samuel Thibault, on sam. 26 août 2017 00:37:21 +0200, wrote: > 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> > Acked-by: Philippe Mathieu-Daudé <f4bug@amsat.org> > --- > slirp/socket.c | 39 +++++++++++++++++++++++---------------- > 1 file changed, 23 insertions(+), 16 deletions(-) > > diff --git a/slirp/socket.c b/slirp/socket.c > index ecec0295a9..cb7b5b608d 100644 > --- a/slirp/socket.c > +++ b/slirp/socket.c > @@ -59,6 +59,27 @@ socreate(Slirp *slirp) > return(so); > } > > +/* > + * Remove references to so from the given message queue. > + */ > +static void > +soqfree(struct socket *so, struct quehead *qh) > +{ > + struct mbuf *ifq; > + > + for (ifq = (struct mbuf *) qh->qh_link; > + (struct quehead *) ifq != qh; > + 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; > + } > + } > + } > +} > + > /* > * remque and free a socket, clobber cache > */ > @@ -66,23 +87,9 @@ 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; > - } > - } > - > - 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; > - } > - } > + soqfree(so, &slirp->if_fastq); > + soqfree(so, &slirp->if_batchq); > > if (so->so_emu==EMU_RSH && so->extra) { > sofree(so->extra); > -- > 2.14.1 > -- Samuel ... <rv_> et Ctrl alt F2 pour aller sous console <rv_> mais c koi pour passer d'un bureau a un autre ! <rv_> au fait c koi le raccourci pour passer d'un bureau a un autre 'question stupide" <cycyx> ça dépend du window manager et de ta conf <Firebird> ce qui fonctionne toujours c'est CTRL-ALT-BCKSP -:- SignOff rv_: #linuxfr (Read error: EOF from client) -:- rv_ [~rv@217.11.166.169] has joined #linuxfr <rv_> Firebird: MEURT...
Samuel Thibault, on sam. 26 août 2017 01:05:04 +0200, wrote: > So Wjjzhang and PJP, can you confirm that this fixes your uses? PJP, can you forward it to Wjjzhang? I keep getting <wjjzhang@tencent.com>: host cloudmx.qq.com[113.108.11.188] said: 550 Mail content denied. http://ascloud.qq.com/cgi-bin/readtemplate?t=anti_spam_errors#n1000726 (in reply to end of DATA command) Samuel
Hello Samuel, +-- On Sat, 26 Aug 2017, Samuel Thibault wrote --+ | So Wjjzhang and PJP, can you confirm that this fixes your uses? Yes, I confirm the patch fixes the use-after-free issue. Thank you so much. -- Prasad J Pandit / Red Hat Product Security Team 47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F
Hi Samuel, On 26.08.2017 00:37, Samuel Thibault wrote: > 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. I think you should CC: this to qemu-stable if it's fixing a problem that can be used by the guest to crash QEMU... ? Thomas > Signed-off-by: Samuel Thibault <samuel.thibault@ens-lyon.org> > Acked-by: Philippe Mathieu-Daudé <f4bug@amsat.org> > --- > slirp/socket.c | 39 +++++++++++++++++++++++---------------- > 1 file changed, 23 insertions(+), 16 deletions(-) > > diff --git a/slirp/socket.c b/slirp/socket.c > index ecec0295a9..cb7b5b608d 100644 > --- a/slirp/socket.c > +++ b/slirp/socket.c > @@ -59,6 +59,27 @@ socreate(Slirp *slirp) > return(so); > } > > +/* > + * Remove references to so from the given message queue. > + */ > +static void > +soqfree(struct socket *so, struct quehead *qh) > +{ > + struct mbuf *ifq; > + > + for (ifq = (struct mbuf *) qh->qh_link; > + (struct quehead *) ifq != qh; > + 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; > + } > + } > + } > +} > + > /* > * remque and free a socket, clobber cache > */ > @@ -66,23 +87,9 @@ 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; > - } > - } > - > - 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; > - } > - } > + soqfree(so, &slirp->if_fastq); > + soqfree(so, &slirp->if_batchq); > > if (so->so_emu==EMU_RSH && so->extra) { > sofree(so->extra); >
Thomas Huth, on mer. 30 août 2017 09:50:45 +0200, wrote: > On 26.08.2017 00:37, Samuel Thibault wrote: > > 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. > > I think you should CC: this to qemu-stable if it's fixing a problem that > can be used by the guest to crash QEMU... ? Indeed. I thought it should first go to master. Samuel > Thomas > > > Signed-off-by: Samuel Thibault <samuel.thibault@ens-lyon.org> > > Acked-by: Philippe Mathieu-Daudé <f4bug@amsat.org> > > --- > > slirp/socket.c | 39 +++++++++++++++++++++++---------------- > > 1 file changed, 23 insertions(+), 16 deletions(-) > > > > diff --git a/slirp/socket.c b/slirp/socket.c > > index ecec0295a9..cb7b5b608d 100644 > > --- a/slirp/socket.c > > +++ b/slirp/socket.c > > @@ -59,6 +59,27 @@ socreate(Slirp *slirp) > > return(so); > > } > > > > +/* > > + * Remove references to so from the given message queue. > > + */ > > +static void > > +soqfree(struct socket *so, struct quehead *qh) > > +{ > > + struct mbuf *ifq; > > + > > + for (ifq = (struct mbuf *) qh->qh_link; > > + (struct quehead *) ifq != qh; > > + 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; > > + } > > + } > > + } > > +} > > + > > /* > > * remque and free a socket, clobber cache > > */ > > @@ -66,23 +87,9 @@ 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; > > - } > > - } > > - > > - 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; > > - } > > - } > > + soqfree(so, &slirp->if_fastq); > > + soqfree(so, &slirp->if_batchq); > > > > if (so->so_emu==EMU_RSH && so->extra) { > > sofree(so->extra); > > > -- Samuel CN > J'ai enseigné l'algorythmique. GLG> C'est quoi l'algorythmique ? Une contrebasse programmée en Algol ? -+- in : Guide du Neuneu d'Usenet - Neuneu fait ses gammes. -+-
On 30 August 2017 at 08:52, Samuel Thibault <samuel.thibault@gnu.org> wrote: > Thomas Huth, on mer. 30 août 2017 09:50:45 +0200, wrote: >> On 26.08.2017 00:37, Samuel Thibault wrote: >> > 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. >> >> I think you should CC: this to qemu-stable if it's fixing a problem that >> can be used by the guest to crash QEMU... ? > > Indeed. I thought it should first go to master. The way our cc-stable process works is that you include a line in the commit message "Cc: qemu-stable@nongnu.org" (in the same place as the signed-off-by and other lines) and also cc the email address when sending the patch; then that will go into the git history in master and can be fished out from there. If you don't include it in the original patch send then whoever puts the pull request together has to include it, so it's helpful to add it from the start if it's pretty definitely a thing that should go into stable. thanks -- PMM
© 2016 - 2024 Red Hat, Inc.