Re: [Qemu-devel] A use-after-free in slirp

["Philippe Mathieu-Daudé" <f4bug@amsat.org>]

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 <samuel.thibault@ens-lyon.org>
> 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 <samuel.thibault@ens-lyon.org>

Acked-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

> 
> 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;
> +      }
>       }
>     }
>   
>