Re: [Qemu-devel] [PATCH] slirp: fixed potential use-after-free of a socket

[Vitaly Chipounov]

Hi,

On 21.02.2013 15:33, Jan Kiszka wrote:
> On 2013-02-15 12:00, Vitaly Chipounov wrote:
>> A socket may still have references to it in various queues
>> at the time it is freed, causing memory corruptions.
> Did you see it in practice? Or is this patch based on code review? What
> will happen if those queued mbufs find their ifq_so NULL?

I have a packet trace that triggers this problem when it is injected
into the guest's NIC.
I am still not quite sure why this happens, but suspect that it could be
caused by malformed/partial TCP/IP streams.

Setting ifq_so to NULL shouldn't be an issue because there seems to be
checks for that (e.g., in if.c).
It doesn't seem to affect normal web browsing/downloading in the guest.

Vitaly

>
>> Signed-off-by: Vitaly Chipounov <address@hidden>
>> ---
>>  slirp/socket.c |   29 +++++++++++++++++++++++++++++
>>  1 file changed, 29 insertions(+)
>>
>> diff --git a/slirp/socket.c b/slirp/socket.c
>> index 77b0c98..8a7adc8 100644
>> --- a/slirp/socket.c
>> +++ b/slirp/socket.c
>> @@ -55,6 +55,33 @@ socreate(Slirp *slirp)
>>    return(so);
>>  }
>>  
>> +/**
>> + * It may happen that a socket is still referenced in various
>> + * mbufs at the time it is freed. Clear all references to the
>> + * socket here.
>> + */
>> +static void soremovefromqueues(struct socket *so)
>> +{
>> +    if (!so->so_queued) {
>> +        return;
>> +    }
>> +
>> +    Slirp *slirp = so->slirp;
>> +    struct mbuf *ifm;
>> +
>> +    for (ifm = slirp->if_fastq.ifq_next; ifm != &slirp->if_fastq; ifm = 
>> ifm->ifq_next) {
> This line and the one below smells like checkpatch.pl wasn't run against
> it. Granted, slirp patches easy make checkpatch upset as the input is
> not properly formatted, but please don't add more violations.
>
>> +        if (ifm->ifq_so == so) {
>> +            ifm->ifq_so = NULL;
>> +        }
>> +    }
>> +
>> +    for (ifm = slirp->if_batchq.ifq_next; ifm != &slirp->if_batchq; ifm = 
>> ifm->ifq_next) {
>> +        if (ifm->ifq_so == so) {
>> +            ifm->ifq_so = NULL;
>> +        }
>> +    }
>> +}
>> +
>>  /*
>>   * remque and free a socket, clobber cache
>>   */
>> @@ -79,6 +106,8 @@ sofree(struct socket *so)
>>    if(so->so_next && so->so_prev)
>>      remque(so);  /* crashes if so is not in a queue */
>>  
>> +  soremovefromqueues(so);
>> +
>>    free(so);
>>  }
>>  
>>
> Sorry for the late feedback,
> Jan
>