From mboxrd@z Thu Jan  1 00:00:00 1970
Received: from eggs.gnu.org ([2001:4830:134:3::10]:52045)
	by lists.gnu.org with esmtp (Exim 4.71)
	(envelope-from <peter.maydell@linaro.org>) id 1Whe9X-0007qN-EV
	for qemu-devel@nongnu.org; Tue, 06 May 2014 08:07:24 -0400
Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)
	(envelope-from <peter.maydell@linaro.org>) id 1Whe9R-0001x0-AX
	for qemu-devel@nongnu.org; Tue, 06 May 2014 08:07:19 -0400
Received: from mail-lb0-f176.google.com ([209.85.217.176]:48662)
	by eggs.gnu.org with esmtp (Exim 4.71)
	(envelope-from <peter.maydell@linaro.org>) id 1Whe9Q-0001vp-Sy
	for qemu-devel@nongnu.org; Tue, 06 May 2014 08:07:13 -0400
Received: by mail-lb0-f176.google.com with SMTP id p9so2230525lbv.7
	for <qemu-devel@nongnu.org>; Tue, 06 May 2014 05:07:12 -0700 (PDT)
MIME-Version: 1.0
In-Reply-To: <20140506115309.GB27709@work-vm>
References: <1398688770-23828-1-git-send-email-peter.maydell@linaro.org>
	<1398688770-23828-8-git-send-email-peter.maydell@linaro.org>
	<20140506115309.GB27709@work-vm>
From: Peter Maydell <peter.maydell@linaro.org>
Date: Tue, 6 May 2014 13:06:51 +0100
Message-ID: <CAFEAcA8Qs6sweZHPa09DuHOD7RM_R+WPF1pQoTkgCGxdYEOcZw@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8
Subject: Re: [Qemu-devel] [PATCH v3 7/7] hw/net/stellaris_enet: Convert to
	vmstate
List-Id: <qemu-devel.nongnu.org>
List-Unsubscribe: <https://lists.nongnu.org/mailman/options/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=unsubscribe>
List-Archive: <http://lists.nongnu.org/archive/html/qemu-devel>
List-Post: <mailto:qemu-devel@nongnu.org>
List-Help: <mailto:qemu-devel-request@nongnu.org?subject=help>
List-Subscribe: <https://lists.nongnu.org/mailman/listinfo/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=subscribe>
To: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
Cc: "Michael S. Tsirkin" <mst@redhat.com>, QEMU Developers <qemu-devel@nongnu.org>, Patch Tracking <patches@linaro.org>

On 6 May 2014 12:53, Dr. David Alan Gilbert <dgilbert@redhat.com> wrote:
> * Peter Maydell (peter.maydell@linaro.org) wrote:
>> +{
>> +    stellaris_enet_state *s = opaque;
>> +    int i;
>> +
>> +    /* Sanitize inbound state. Note that next_packet is an index but
>> +     * np is a size; hence their valid upper bounds differ.
>> +     */
>> +    if (s->next_packet >= ARRAY_SIZE(s->rx)) {
>> +        return -1;
>> +    }
>> +
>> +    if (s->np > ARRAY_SIZE(s->rx)) {
>> +        return -1;
>> +    }
>> +
>> +    for (i = 0; i < ARRAY_SIZE(s->rx); i++) {
>> +        if (s->rx[i].len > ARRAY_SIZE(s->rx[i].data)) {
>> +            return -1;
>> +        }
>> +    }
>> +
>> +    if (s->rx_fifo_offset > ARRAY_SIZE(s->rx[0].data) + 4) {
>> +        return -1;
>> +    }
>
> Can you explain that +4 ?
> I think I can see how it would end up equalling ARRAY_SIZE if
> you've just read the last 4 bytes, but how does it go beyond?

Whoops, I think this should be - 4, not + 4 (I think I
messed up when I rearranged this from "offset + 4 > ARRAY_SIZE"
to avoid the potential overflow in that expression.)
The DATA read code is going to read from the 4 bytes starting
at s->rx[s->next_packet].data + s->rx_fifo_offset, so
we need to make sure the offset doesn't allow that to
overrun. (When we read the last 4 bytes then
the rx_fifo_offset is reset to zero immediately, so at
migration it's never possible for it to be equal to
ARRAY_SIZE).

>> +
>> +    if (s->tx_fifo_len > ARRAY_SIZE(s->tx_fifo)) {
>> +        return -1;
>> +    }
>> +
>> +    return 0;
>> +}
>> +
>> +static const VMStateDescription vmstate_stellaris_enet = {
>> +    .name = "stellaris_enet",
>> +    .version_id = 2,
>> +    .minimum_version_id = 2,
>> +    .minimum_version_id_old = 2,
>
> Weren't we killing off the minimum_version_id_old's ?

Yes, but we can't til the patch making it optional
hits master (it is in the current migration pullreq,
so if that goes in OK I'll just delete the _old line.)

thanks
-- PMM