[ipxe-devel] [PATCH] Validate L2 address in DHCP packets.

[Michael Brown]

On 07/09/14 15:29, Wissam Shoukair wrote:
> Checking for valid XID is not enough, because if two PXE clients
> sends the DHCP Discover with the same XID then both of them will get
> both offers while one of them with the wrong client HW address.

The XID should not be duplicated across two nodes; that in itself would 
be an error.  Is this actually happening in practice?

> Signed-off-by: Wissam Shoukair <wissams at mellanox.com>
> ---
>   src/net/udp/dhcp.c | 21 +++++++++++++++++++++
>   1 file changed, 21 insertions(+)
>
> diff --git a/src/net/udp/dhcp.c b/src/net/udp/dhcp.c
> index e6d3edd..d89728f 100644
> --- a/src/net/udp/dhcp.c
> +++ b/src/net/udp/dhcp.c
> @@ -1157,10 +1157,12 @@ static int dhcp_deliver ( struct dhcp_session *dhcp,
>   	size_t data_len;
>   	struct dhcp_packet *dhcppkt;
>   	struct dhcphdr *dhcphdr;
> +	uint8_t local_mac[MAX_LL_ADDR_LEN];

Should probably be

	uint8_t chaddr[ sizeof ( dhcphdr->chaddr ) ];

since the buffer is to hold a DHCP chaddr rather than a MAC address.

>   	uint8_t msgtype = 0;
>   	struct in_addr server_id = { 0 };
>   	int rc = 0;
>
> +

Extra blank line probably not needed.

>   	/* Sanity checks */
>   	if ( ! meta->src ) {
>   		DBGC ( dhcp, "DHCP %p received packet without source port\n",
> @@ -1193,6 +1195,24 @@ static int dhcp_deliver ( struct dhcp_session *dhcp,
>   	dhcppkt_fetch ( dhcppkt, DHCP_SERVER_IDENTIFIER,
>   			&server_id, sizeof ( server_id ) );
>
> +	/* Validate L2 address */
> +#define MAC_FMT		"%02x:%02x:%02x:%02x:%02x:%02x"
> +#define MAC_PTR(x)	((uint8_t*)(x))[0], ((uint8_t*)(x))[1], ((uint8_t*)(x))[2], \
> +					((uint8_t*)(x))[3], ((uint8_t*)(x))[4], ((uint8_t*)(x))[5]

The chaddr is not necessarily an Ethernet MAC.  Probably best to define 
a function dhcp_chaddr_ntoa() (modelled on eth_ntoa() and similar 
functions) which can handle a variable-length chaddr.

> +	dhcp_chaddr ( dhcp->netdev, local_mac, NULL );
> +	DBGC ( dhcp, "DHCP %p %s from %s:%d is for MAC ["MAC_FMT"]."
> +		   "Local MAC is ["MAC_FMT"]. ", dhcp, dhcp_msgtype_name ( msgtype ),
> +		   inet_ntoa ( peer->sin_addr ), ntohs ( peer->sin_port ),
> +		   MAC_PTR( dhcppkt->dhcphdr->chaddr ), MAC_PTR( local_mac ) );
> +	if ( memcmp ( dhcppkt->dhcphdr->chaddr, local_mac, dhcppkt->dhcphdr->hlen ) ) {
> +		DBGC ( dhcp, "Packet discarded.\n" );
> +		rc = -EINVAL;
> +		goto err_mac;
> +	} else {
> +		DBGC ( dhcp, "Handling packet.\n" );
> +	}

Please match the style of other debugging messages in the same 
file/function.  For example, the pattern of the other debug messages in 
dhcp_deliver() is to produce output only on discovering a reason to 
reject the packet, and the general format is

   DBGC ( dhcp, "DHCP %p %s ....", dhcp, dhcp_msgtype_name ( msgtype ), 
... );

Thanks,

Michael