Memory corruption in #ifdef HAVE_PACKET_AUXDATA when packet is received with more than 1 VLAN Tag

[nikchez01]

File lib/netdev.c:

/* VLAN tag found. Shift MAC addresses down and insert VLAN tag /
/ Create headroom for the VLAN tag /
eth_type = ntohs(((uint16_t )(buffer->data + ETHER_ADDR_LEN * 2)));
ofpbuf_push_uninit(buffer, VLAN_HEADER_LEN);
memmove(buffer->data, (uint8_t)buffer->data+VLAN_HEADER_LEN, ETH_ALEN * 2);

Mem move ofset is wrong. It should be 2 bytes shifted as we need to consider ETH TYPE field also:

Correction:
memmove(buffer->data+VLAN_ETH_HEADER_LEN-2, (uint8_t*)buffer->data+ETH_HEADER_LEN-2, buffer->size-(ETHER_ADDR_LEN-2));

[ederlf]

I do not know if I understand what do you mean.

ofpbuf_push_uninit(buffer, VLAN_HEADER_LEN)
Allocates room for the VLAN header.

memmove(buffer->data, (uint8_t*)buffer->data+VLAN_HEADER_LEN, ETH_ALEN * 2)

The dst and src MAC address need to be moved to the new head. The new head is in an address that points to the previous address - VLAN_HEADER_LEN. So memmove takes the mac addresses from the previous position which is exactly VLAN_HEADER_LEN and moves them to the head.

The it is just a matter of filling the tag, which is done by pointing the VLAN position to the end of the MAC address.

tag = (struct vlan_tag *)((uint8_t*)buffer->data + ETH_ALEN * 2);

The ethertype comes after the VLAN tag, so it is left after the vlan tag.

[nikchez01]

I understand your point @ederlf .
However, above logic will fail in case packet has more than 1 vlan tag (say 2 tags as a practical situation). My testing scenario consisted both C-VLAN and S-VLAN

In case of 2 Vlan tags, outer VLAN will become inner VLAN while copying.

So instead of adding memory in front, it is better to add memory to the last (or just before last ethertype)

Also, there is some issue in my above changes. If we want to create space in middle or end then we have to use ofpbuf_put_uninit instead of push.

I may be wrong but I think I heard somewhere that linux does not support more than 1 VLAN. But still it is better to change code if any such support is present or added in future.

[ederlf]

@nikchez01 I know you want to help and I do really appreciate and understand you could possibly be doing it in your free time (as do I), but I have to ask you to be more specific when reporting a problem.

How could I guess, from the first report, it fails when there are two vlans? When submitting an issue please try to be as complete as possible. ≈

• What exactly is the problem? I understand it is hard to come up with good titles for the issues (Commits are the same case), but notice it is a VLAN problem, but the title does not mention anything about it. I know HAVE_PACKET_AUXDATA is related to VLAN so I could go directly to it, but what if another user is searching a solution for the same problem? An example of a more fitting title would be:
  "Memory corruption when receiving packets with more than 1 VLAN tag."
• When it happens?
• Output of the problem. If you can show the logs, that could lead to faster debugging.
• How it happens? If possible, the exact steps to reproduce the error.
• Optional: Where it happens? It is amazing if you can point at the code where the problem is, but I would never expect this.

Also, if you mention a solution, tell already it might have an issue and that you would like some help to solve it.

[nikchez01]

My apologies @ederlf . I will keep above points in mind while posting further issues.