Re: CVS commit: src/sys/dev/hyperv

[Taylor R Campbell <riastradh%NetBSD.org@localhost>]

> Module Name:    src
> Committed By:   joe
> Date:           Sun Feb 16 15:56:06 UTC 2025
> 
> Modified Files:
>         src/sys/dev/hyperv: if_hvn.c
> 
> Log Message:
> avoid buffer overflow when copying the ether header host into the ether vland header host
> on hyper-v
> [...]
> @@ -1697,7 +1697,7 @@ hvn_bpf_mtap(struct hvn_tx_ring *txr, st
>  	*/
>  
>  	eh = mtod(m, struct ether_header *);
> -	memcpy(evl.evl_dhost, eh->ether_dhost, ETHER_ADDR_LEN * 2);
> +	memcpy(evl.evl_dhost, eh->ether_dhost, ETHER_ADDR_LEN);
>  	evl.evl_encap_proto = htons(ETHERTYPE_VLAN);
>  	evl.evl_tag = htons(vlan_get_tag(m));
>  	evl.evl_proto = eh->ether_type;
> @@ -4836,7 +4836,7 @@ hvn_rxeof(struct hvn_rx_ring *rxr, uint8
>  
>  			evl = mtod(m, struct ether_vlan_header *);
>  			memcpy(evl->evl_dhost, eh.ether_dhost,
> -			    ETHER_ADDR_LEN * 2);
> +			    ETHER_ADDR_LEN);

I think this change is not correct and should be either reverted or
done differently.  There is no buffer overrun here because of the
shape of struct ether_vlan_header and struct ether_header:

struct ether_vlan_header {
	uint8_t		evl_dhost[ETHER_ADDR_LEN];
	uint8_t		evl_shost[ETHER_ADDR_LEN];
	uint16_t	evl_encap_proto;
	uint16_t	evl_tag;
	uint16_t	evl_proto;
} __packed;

struct ether_header {
	uint8_t  ether_dhost[ETHER_ADDR_LEN];
	uint8_t  ether_shost[ETHER_ADDR_LEN];
	uint16_t ether_type;
};

So there are two consecutive ETHER_ADDR_LEN-byte arrays starting at
evl.evl_dhost and eh->ether_dhost, and we need to copy _both_ of them
(the destination and source addresses).

It is likely that static analyzers don't like this because, without
the context, it looks like a buffer overrun: the input to memcpy is
formally an array of length ETHER_ADDR_LEN.

But since there is no padding in either structure[*], there is another
array of length ETHER_ADDR_LEN consecutive in memory, so this is not
actually a buffer overrun.  And copying only the destination address,
not the source address, will certainly break this driver.

If you want to pacify the static analyzers without breaking the
functionality, I suggest writing two memcpy calls:

	memcpy(evl.evl_dhost, eh->ether_dhost, ETHER_ADDR_LEN);
	memcpy(evl.evl_shost, eh->ether_shost, ETHER_ADDR_LEN);

Just be careful of applying the suggestions of static analyzers --
they are not always right!  I suggest you ask for review before making
changes like this, especially if you don't have the hardware (or, in
this case, host software) to test the driver.  You can file a PR with
the proposed change to help keep track -- and that will also help to
track pullups in case netbsd-9 or netbsd-10 also need fixes.


[*] struct ether_header used to be marked __packed, but roy@ removed
    that back in 2021:

    https://mail-index.netbsd.org/source-changes/2021/02/03/msg126553.html

    He put a __CTASSERT of the size in to ensure the build fails if
    the compiler ever inserts any padding (I think I may have
    requested that):

    https://mail-index.netbsd.org/source-changes/2021/02/03/msg126567.html
    https://mail-index.netbsd.org/source-changes/2021/02/03/msg126578.html

    But then removed it while removing some alignment checks, for
    reasons unclear to me:

    https://mail-index.netbsd.org/source-changes/2021/02/14/msg126887.html

    In any case, whether or not we have a static assertion to verify
    the size, none of our ethernet drivers would function if struct
    ether_header had any padding in it!