On 6 May 2014 12:53, Dr. David Alan Gilbert <dgilb...@redhat.com> wrote:
> * Peter Maydell (peter.mayd...@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
