Re: [Qemu-devel] [Slirp] [PATCH 1/2] Do not reassemble fragments pointing outside of the original payload

[Samuel Thibault <samuel.thibault@gnu.org>]

Hello,

Philippe Mathieu-Daudé, le jeu. 22 août 2019 16:41:33 +0200, a ecrit:
>   Later the newly calculated pointer q is converted into ip structure
>   and values are modified, Due to the wrong calculation of the delta,
>   ip will be pointing to incorrect location and ip_src and ip_dst can
>   be used to write controlled data onto the calculated location. This
>   may also crash qemu if the calculated ip is located in unmaped area.

That does not seem to be related to this:

> Do not queue fragments pointing out of the original payload to avoid
> to calculate the variable delta.

I don't understand the relation with having to calculate delta.

> diff --git a/src/ip_input.c b/src/ip_input.c
> index 7364ce0..ee52085 100644
> --- a/src/ip_input.c
> +++ b/src/ip_input.c
> @@ -304,6 +304,19 @@ static struct ip *ip_reass(Slirp *slirp, struct ip *ip, struct ipq *fp)
>          ip_deq(q->ipf_prev);
>      }
>  
> +    /*
> +     * If we received the first fragment, we know the original
> +     * payload size.

? We only know the total payload size when receiving the last fragment
(payload = offset*8 + size).

> Verify fragments are within our payload.

By construction of the protocol, fragments can only be within the
payload, since it's the last fragment which provides the payload size.

> +    for (q = fp->frag_link.next; q != (struct ipasfrag*)&fp->frag_link;
> +            q = q->ipf_next) {
> +        if (!q->ipf_off && q->ipf_len) {
> +            if (ip->ip_off + ip->ip_len >= q->ipf_len) {
> +                goto dropfrag;
> +            }
> +        }
> +    }

Fragments are kept in order, there is no need to go around the list to
find the fragment with offset zero, if it is there it is the first one.

Did you make your test with commit 126c04acbabd ("Fix heap overflow in
ip_reass on big packet input") applied?

Samuel