[PATCH net-next] vxlan: drop packets from invalid src-address

[PATCH net-next] vxlan: drop packets from invalid src-address

[David Bauer]

The VXLAN driver currently does not check if the inner layer2
source-address is valid.

In case source-address snooping/learning is enabled, a entry in the FDB
for the invalid address is created with the layer3 address of the tunnel
endpoint.

If the frame happens to have a non-unicast address set, all this
non-unicast traffic is subsequently not flooded to the tunnel network
but sent to the learnt host in the FDB. To make matters worse, this FDB
entry does not expire.

Apply the same filtering for packets as it is done for bridges. This not
only drops these invalid packets but avoids them from being learnt into
the FDB.

Suggested-by: Ido Schimmel <idosch@nvidia.com>
Signed-off-by: David Bauer <mail@david-bauer.net>
---
 drivers/net/vxlan/vxlan_core.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/net/vxlan/vxlan_core.c b/drivers/net/vxlan/vxlan_core.c
index 3495591a5c29..ba319fc21957 100644
--- a/drivers/net/vxlan/vxlan_core.c
+++ b/drivers/net/vxlan/vxlan_core.c
@@ -1615,6 +1615,10 @@ static bool vxlan_set_mac(struct vxlan_dev *vxlan,
 	if (ether_addr_equal(eth_hdr(skb)->h_source, vxlan->dev->dev_addr))
 		return false;
 
+	/* Ignore packets from invalid src-address */
+	if (!is_valid_ether_addr(eth_hdr(skb)->h_source))
+		return false;
+
 	/* Get address from the outer IP header */
 	if (vxlan_get_sk_family(vs) == AF_INET) {
 		saddr.sin.sin_addr.s_addr = ip_hdr(skb)->saddr;
-- 
2.43.0

Re: [PATCH net-next] vxlan: drop packets from invalid src-address

[Ratheesh Kannoth]

On 2024-04-01 at 02:44:34, David Bauer (mail@david-bauer.net) wrote:
> The VXLAN driver currently does not check if the inner layer2
> source-address is valid.
>
> In case source-address snooping/learning is enabled, a entry in the FDB
> for the invalid address is created with the layer3 address of the tunnel
> endpoint.
what is root cause of creation of invalid MAC from an L3 address ? could you
add that as well to commit message.

>
> If the frame happens to have a non-unicast address set, all this
> non-unicast traffic is subsequently not flooded to the tunnel network
> but sent to the learnt host in the FDB. To make matters worse, this FDB
> entry does not expire.
>
> Apply the same filtering for packets as it is done for bridges. This not
> only drops these invalid packets but avoids them from being learnt into
> the FDB.
>
> Suggested-by: Ido Schimmel <idosch@nvidia.com>
> Signed-off-by: David Bauer <mail@david-bauer.net>
> ---
>  drivers/net/vxlan/vxlan_core.c | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/drivers/net/vxlan/vxlan_core.c b/drivers/net/vxlan/vxlan_core.c
> index 3495591a5c29..ba319fc21957 100644
> --- a/drivers/net/vxlan/vxlan_core.c
> +++ b/drivers/net/vxlan/vxlan_core.c
> @@ -1615,6 +1615,10 @@ static bool vxlan_set_mac(struct vxlan_dev *vxlan,
>  	if (ether_addr_equal(eth_hdr(skb)->h_source, vxlan->dev->dev_addr))
>  		return false;
>
> +	/* Ignore packets from invalid src-address */
> +	if (!is_valid_ether_addr(eth_hdr(skb)->h_source))
> +		return false;
> +
>  	/* Get address from the outer IP header */
>  	if (vxlan_get_sk_family(vs) == AF_INET) {
>  		saddr.sin.sin_addr.s_addr = ip_hdr(skb)->saddr;
> --
> 2.43.0
>

Re: [PATCH net-next] vxlan: drop packets from invalid src-address

[David Bauer]

Hello Ratheesh,

On 4/1/24 05:04, Ratheesh Kannoth wrote:
> On 2024-04-01 at 02:44:34, David Bauer (mail@david-bauer.net) wrote:
>> The VXLAN driver currently does not check if the inner layer2
>> source-address is valid.
>>
>> In case source-address snooping/learning is enabled, a entry in the FDB
>> for the invalid address is created with the layer3 address of the tunnel
>> endpoint.
> what is root cause of creation of invalid MAC from an L3 address ? could you
> add that as well to commit message.

I sadly can not elaborate on this further as the state happens sporadically
after weeks of operation. For more details, see

https://lore.kernel.org/all/15ee0cc7-9252-466b-8ce7-5225d605dde8@david-bauer.net/T/

Best
David

> 
>>
>> If the frame happens to have a non-unicast address set, all this
>> non-unicast traffic is subsequently not flooded to the tunnel network
>> but sent to the learnt host in the FDB. To make matters worse, this FDB
>> entry does not expire.
>>
>> Apply the same filtering for packets as it is done for bridges. This not
>> only drops these invalid packets but avoids them from being learnt into
>> the FDB.
>>
>> Suggested-by: Ido Schimmel <idosch@nvidia.com>
>> Signed-off-by: David Bauer <mail@david-bauer.net>
>> ---
>>   drivers/net/vxlan/vxlan_core.c | 4 ++++
>>   1 file changed, 4 insertions(+)
>>
>> diff --git a/drivers/net/vxlan/vxlan_core.c b/drivers/net/vxlan/vxlan_core.c
>> index 3495591a5c29..ba319fc21957 100644
>> --- a/drivers/net/vxlan/vxlan_core.c
>> +++ b/drivers/net/vxlan/vxlan_core.c
>> @@ -1615,6 +1615,10 @@ static bool vxlan_set_mac(struct vxlan_dev *vxlan,
>>   	if (ether_addr_equal(eth_hdr(skb)->h_source, vxlan->dev->dev_addr))
>>   		return false;
>>
>> +	/* Ignore packets from invalid src-address */
>> +	if (!is_valid_ether_addr(eth_hdr(skb)->h_source))
>> +		return false;
>> +
>>   	/* Get address from the outer IP header */
>>   	if (vxlan_get_sk_family(vs) == AF_INET) {
>>   		saddr.sin.sin_addr.s_addr = ip_hdr(skb)->saddr;
>> --
>> 2.43.0
>>

Re: [PATCH net-next] vxlan: drop packets from invalid src-address

[Simon Horman]

On Sun, Mar 31, 2024 at 11:14:34PM +0200, David Bauer wrote:
> The VXLAN driver currently does not check if the inner layer2
> source-address is valid.
> 
> In case source-address snooping/learning is enabled, a entry in the FDB
> for the invalid address is created with the layer3 address of the tunnel
> endpoint.
> 
> If the frame happens to have a non-unicast address set, all this
> non-unicast traffic is subsequently not flooded to the tunnel network
> but sent to the learnt host in the FDB. To make matters worse, this FDB
> entry does not expire.
> 
> Apply the same filtering for packets as it is done for bridges. This not
> only drops these invalid packets but avoids them from being learnt into
> the FDB.
> 
> Suggested-by: Ido Schimmel <idosch@nvidia.com>
> Signed-off-by: David Bauer <mail@david-bauer.net>

Hi David and Ido,

I wonder if this is an appropriate candidate for 'net', with a Fixes tag.
It does seem to address a user-visible problem.

...

Re: [PATCH net-next] vxlan: drop packets from invalid src-address

[Ido Schimmel]

On Tue, Apr 02, 2024 at 07:08:48PM +0100, Simon Horman wrote:
> On Sun, Mar 31, 2024 at 11:14:34PM +0200, David Bauer wrote:
> > The VXLAN driver currently does not check if the inner layer2
> > source-address is valid.
> > 
> > In case source-address snooping/learning is enabled, a entry in the FDB
> > for the invalid address is created with the layer3 address of the tunnel
> > endpoint.
> > 
> > If the frame happens to have a non-unicast address set, all this
> > non-unicast traffic is subsequently not flooded to the tunnel network
> > but sent to the learnt host in the FDB. To make matters worse, this FDB
> > entry does not expire.
> > 
> > Apply the same filtering for packets as it is done for bridges. This not
> > only drops these invalid packets but avoids them from being learnt into
> > the FDB.
> > 
> > Suggested-by: Ido Schimmel <idosch@nvidia.com>
> > Signed-off-by: David Bauer <mail@david-bauer.net>
> 
> Hi David and Ido,
> 
> I wonder if this is an appropriate candidate for 'net', with a Fixes tag.
> It does seem to address a user-visible problem.

I'm OK with targeting the patch at 'net'. Looking at git history, the
issue seems to be present since initial submission so Fixes tag should
be:

Fixes: d342894c5d2f ("vxlan: virtual extensible lan")

David, can you please re-submit with "[PATCH net]" prefix and the above
tag?

Thanks

Re: [PATCH net-next] vxlan: drop packets from invalid src-address

[David Bauer]

Hi Ido,

On 4/3/24 14:45, Ido Schimmel wrote:
> On Tue, Apr 02, 2024 at 07:08:48PM +0100, Simon Horman wrote:
>> On Sun, Mar 31, 2024 at 11:14:34PM +0200, David Bauer wrote:
>>> The VXLAN driver currently does not check if the inner layer2
>>> source-address is valid.
>>>
>>> In case source-address snooping/learning is enabled, a entry in the FDB
>>> for the invalid address is created with the layer3 address of the tunnel
>>> endpoint.
>>>
>>> If the frame happens to have a non-unicast address set, all this
>>> non-unicast traffic is subsequently not flooded to the tunnel network
>>> but sent to the learnt host in the FDB. To make matters worse, this FDB
>>> entry does not expire.
>>>
>>> Apply the same filtering for packets as it is done for bridges. This not
>>> only drops these invalid packets but avoids them from being learnt into
>>> the FDB.
>>>
>>> Suggested-by: Ido Schimmel <idosch@nvidia.com>
>>> Signed-off-by: David Bauer <mail@david-bauer.net>
>>
>> Hi David and Ido,
>>
>> I wonder if this is an appropriate candidate for 'net', with a Fixes tag.
>> It does seem to address a user-visible problem.
> 
> I'm OK with targeting the patch at 'net'. Looking at git history, the
> issue seems to be present since initial submission so Fixes tag should
> be:
> 
> Fixes: d342894c5d2f ("vxlan: virtual extensible lan")
> 
> David, can you please re-submit with "[PATCH net]" prefix and the above
> tag?

I can take care of that. Thanks for analyzing the situation.

One thing i still have in my head when looking at this:

 From my understanding, when i manage to send out such a packet from e.g. a
VM connected to a vxlan overlay network and manage to send out such malformed
packet, this would allow me to break the overlay network created with vxlan doesn't it?

Can you comment on my assumption there? I assume you have a better understanding of
the inner workings of the vxlan protocol.

Best
David

> 
> Thanks

Re: [PATCH net-next] vxlan: drop packets from invalid src-address

[Ido Schimmel]

On Wed, Apr 03, 2024 at 07:14:14PM +0200, David Bauer wrote:
> I can take care of that. Thanks for analyzing the situation.
> 
> One thing i still have in my head when looking at this:
> 
> From my understanding, when i manage to send out such a packet from e.g. a
> VM connected to a vxlan overlay network and manage to send out such malformed
> packet, this would allow me to break the overlay network created with vxlan doesn't it?
> 
> Can you comment on my assumption there?

I'm not sure which assumption you are referring to, but I did verify
that before your patch the VXLAN driver will learn an FDB entry with a
broadcast MAC if a malformed packet with a broadcast source MAC was
processed by it. This will cause the driver to send broadcast packets to
the VTEP that sent the malformed packet instead of flooding such packets
to all the VTEPs via the all-zeroes FDB entry. This behavior is
obviously wrong and I tested that it doesn't happen with your patch.