> 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!
