[systemd-devel] [PATCH] sd-dhcp-client: support non-Ethernet hardware addresses

[Patrik Flykt]

	Hi,

On Fri, 2014-09-26 at 15:15 -0500, Dan Williams wrote:
>          /* RFC2132 section 4.1.1:
>             The client MUST include its hardware address in the ’chaddr’ field, if
> -           necessary for delivery of DHCP reply messages.
> +           necessary for delivery of DHCP reply messages.  Non-Ethernet
> +           interfaces will leave 'chaddr' empty and use the client identifier
> +           instead (eg, RFC 4390 section 2.1).
>           */
> -        memcpy(&packet->dhcp.chaddr, &client->client_id.mac_addr, ETH_ALEN);
> +        if (client->mac_addr_len == ETH_ALEN)
> +                memcpy(&packet->dhcp.chaddr, &client->mac_addr, ETH_ALEN);

Sorry about the late review, but shouldn't this be more generic if
written:
	if (client->mac_addr_len)
		memcpy(&packet->dhcp.chaddr, &client->mac_addr, client->mac_addr_len);

With that, all cases are covered. Which, AFAIK, are none in addition to
ethernet. An assert() should check the given MAC address length at some
point in the code so that it cannot be > 16.

...

> diff --git a/src/libsystemd-network/sd-dhcp-server.c b/src/libsystemd-network/sd-dhcp-server.c
> index a6d6178..24fedd2 100644
> --- a/src/libsystemd-network/sd-dhcp-server.c
> +++ b/src/libsystemd-network/sd-dhcp-server.c
> @@ -388,16 +388,16 @@ static int server_message_init(sd_dhcp_server *server, DHCPPacket **ret,
>          assert(IN_SET(type, DHCP_OFFER, DHCP_ACK, DHCP_NAK));
>  
>          packet = malloc0(sizeof(DHCPPacket) + req->max_optlen);
>          if (!packet)
>                  return -ENOMEM;
>  
>          r = dhcp_message_init(&packet->dhcp, BOOTREPLY,
> -                              be32toh(req->message->xid), type, req->max_optlen,
> -                              &optoffset);
> +                              be32toh(req->message->xid), type, ARPHRD_ETHER,
> +                              req->max_optlen, &optoffset);
>          if (r < 0)
>                  return r;
>  
>          packet->dhcp.flags = req->message->flags;
>          packet->dhcp.giaddr = req->message->giaddr;
>          memcpy(&packet->dhcp.chaddr, &req->message->chaddr, ETH_ALEN);

Unrelated to your patch, I think a bug is lurking on the last line. I
believe the full 16 bytes of dhcp.chaddr should be copied from the
client's message according to the table in Section 4.3.1 in RFC 2131.
Including any possible garbage the client left in its message. I wonder
how many client implementation break due to such a change, though...

Cheers,

	Patrik