* Re: [Qemu-devel] [PATCH] slirp: fix clearing ifq_so from pending packets
2017-08-25 22:37 [Qemu-devel] [PATCH] slirp: fix clearing ifq_so from pending packets Samuel Thibault
@ 2017-08-25 22:45 ` Philippe Mathieu-Daudé
2017-08-25 23:05 ` Samuel Thibault
2017-08-30 7:50 ` Thomas Huth
2 siblings, 0 replies; 8+ messages in thread
From: Philippe Mathieu-Daudé @ 2017-08-25 22:45 UTC (permalink / raw)
To: Samuel Thibault, qemu-devel; +Cc: thuth, jan.kiszka, ppandit, wjjzhang
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);
>
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [Qemu-devel] [PATCH] slirp: fix clearing ifq_so from pending packets
2017-08-25 22:37 [Qemu-devel] [PATCH] slirp: fix clearing ifq_so from pending packets Samuel Thibault
2017-08-25 22:45 ` Philippe Mathieu-Daudé
@ 2017-08-25 23:05 ` Samuel Thibault
2017-08-25 23:07 ` Samuel Thibault
2017-08-28 10:01 ` P J P
2017-08-30 7:50 ` Thomas Huth
2 siblings, 2 replies; 8+ messages in thread
From: Samuel Thibault @ 2017-08-25 23:05 UTC (permalink / raw)
To: qemu-devel; +Cc: f4bug, ppandit, jan.kiszka, thuth, wjjzhang
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...
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [Qemu-devel] [PATCH] slirp: fix clearing ifq_so from pending packets
2017-08-25 23:05 ` Samuel Thibault
@ 2017-08-25 23:07 ` Samuel Thibault
2017-08-28 10:01 ` P J P
1 sibling, 0 replies; 8+ messages in thread
From: Samuel Thibault @ 2017-08-25 23:07 UTC (permalink / raw)
To: qemu-devel; +Cc: f4bug, ppandit, jan.kiszka, thuth, wjjzhang
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
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [Qemu-devel] [PATCH] slirp: fix clearing ifq_so from pending packets
2017-08-25 23:05 ` Samuel Thibault
2017-08-25 23:07 ` Samuel Thibault
@ 2017-08-28 10:01 ` P J P
1 sibling, 0 replies; 8+ messages in thread
From: P J P @ 2017-08-28 10:01 UTC (permalink / raw)
To: Samuel Thibault; +Cc: qemu-devel, f4bug, jan.kiszka, thuth, wjjzhang
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH] slirp: fix clearing ifq_so from pending packets
2017-08-25 22:37 [Qemu-devel] [PATCH] slirp: fix clearing ifq_so from pending packets Samuel Thibault
2017-08-25 22:45 ` Philippe Mathieu-Daudé
2017-08-25 23:05 ` Samuel Thibault
@ 2017-08-30 7:50 ` Thomas Huth
2017-08-30 7:52 ` Samuel Thibault
2 siblings, 1 reply; 8+ messages in thread
From: Thomas Huth @ 2017-08-30 7:50 UTC (permalink / raw)
To: Samuel Thibault, qemu-devel
Cc: jan.kiszka, f4bug, ppandit, wjjzhang, qemu-stable
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);
>
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [Qemu-devel] [PATCH] slirp: fix clearing ifq_so from pending packets
2017-08-30 7:50 ` Thomas Huth
@ 2017-08-30 7:52 ` Samuel Thibault
2017-08-30 11:30 ` Peter Maydell
0 siblings, 1 reply; 8+ messages in thread
From: Samuel Thibault @ 2017-08-30 7:52 UTC (permalink / raw)
To: Thomas Huth; +Cc: qemu-devel, jan.kiszka, f4bug, ppandit, wjjzhang, qemu-stable
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. -+-
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [Qemu-devel] [PATCH] slirp: fix clearing ifq_so from pending packets
2017-08-30 7:52 ` Samuel Thibault
@ 2017-08-30 11:30 ` Peter Maydell
0 siblings, 0 replies; 8+ messages in thread
From: Peter Maydell @ 2017-08-30 11:30 UTC (permalink / raw)
To: Samuel Thibault
Cc: Thomas Huth, Jan Kiszka, qemu-stable, QEMU Developers, P J P,
Wjjzhang, Philippe Mathieu-Daudé
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
^ permalink raw reply [flat|nested] 8+ messages in thread