From mboxrd@z Thu Jan  1 00:00:00 1970
Authentication-Results: passt.top; dmarc=pass (p=none dis=none) header.from=redhat.com
Authentication-Results: passt.top;
	dkim=pass (1024-bit key; unprotected) header.d=redhat.com header.i=@redhat.com header.a=rsa-sha256 header.s=mimecast20190719 header.b=K06nSmAc;
	dkim-atps=neutral
Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124])
	by passt.top (Postfix) with ESMTPS id 0D7255A0274
	for <passt-dev@passt.top>; Thu, 16 Jan 2025 01:31:07 +0100 (CET)
DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com;
	s=mimecast20190719; t=1736987466;
	h=from:from:reply-to:subject:subject:date:date:message-id:message-id:
	 to:to:cc:cc:mime-version:mime-version:content-type:content-type:
	 content-transfer-encoding:content-transfer-encoding:
	 in-reply-to:in-reply-to:references:references;
	bh=VfLmTl9Bbd0/a2c5YN4LPkBRJMM7/LuZHvqVFiO01UE=;
	b=K06nSmAclC3WaOF++3LIbiR3GInM/XBCC+Oz3Mx4W8p8hci0WIlN0uap83AFu0rOBgyDUM
	b9Cx4vIFxdSx18bvYE5yU9e0uMEOBwXgQzu6SaOYVUONM/AUKHlla3jOSoQrvUujrbDnnx
	ER3QXdtN88Lt5rzKMWN9N62W0mqSHd8=
Received: from mail-wr1-f71.google.com (mail-wr1-f71.google.com
 [209.85.221.71]) by relay.mimecast.com with ESMTP with STARTTLS
 (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id
 us-mta-134-SwGfO2HlPgmdpL9OsT86wA-1; Wed, 15 Jan 2025 19:31:05 -0500
X-MC-Unique: SwGfO2HlPgmdpL9OsT86wA-1
X-Mimecast-MFC-AGG-ID: SwGfO2HlPgmdpL9OsT86wA
Received: by mail-wr1-f71.google.com with SMTP id ffacd0b85a97d-38a873178f2so181084f8f.1
        for <passt-dev@passt.top>; Wed, 15 Jan 2025 16:31:04 -0800 (PST)
X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed;
        d=1e100.net; s=20230601; t=1736987463; x=1737592263;
        h=content-transfer-encoding:mime-version:organization:references
         :in-reply-to:message-id:subject:cc:to:from:date:x-gm-message-state
         :from:to:cc:subject:date:message-id:reply-to;
        bh=VfLmTl9Bbd0/a2c5YN4LPkBRJMM7/LuZHvqVFiO01UE=;
        b=q+/JblLCac30XFdT13JmZsb5j+6EPzteEdzsxVwCkzpeotUaYjBJMdb9VH3FUfDYYQ
         R8ldLB6oI/IJLtCLfL0LsvX+sOEvcuT8fyw818ntV7FVqQLlkvG1GIp9RhqFDlSm2i2H
         Pk457x7dG9sOs6jCcNuB1BGbUfaUsYz0zTEKVIxaWPvCXgqPap7ycwURXbUTo76da+Fw
         qefWHWNpgmmzQdsC1/JLH/hlIbslbMUeTjfdZobQlkrs/vFmD1zteiRsft98UxmUdOpc
         ETwQ6nkyDFVDu5B3VeLfW1BtOPtUKeeC5b0JaZ9qITnk6l1azdv+lW5Im7R7JXrKPSl+
         +2QA==
X-Gm-Message-State: AOJu0YxalrmZM0qsWE25rlR/SU/wn4x/793MkbvtwxVLizNsVfqx3SUI
	04nj7TfPWUj4uZkQ1tWTVpBDcbLVhFuS806MROv5vXF+500Zg0jr1USf+C0a5DygXznokPl7/SQ
	Jf5C4SLO+T59KU3R7n0ebWM43EttZmUcahDNhpgt/ZzX2asBZp3U6uX98vYRc34JmCvXy2CHSJS
	C8f3PjkifQ7gDyG4sQpNRM+hLjs96u5+Yh
X-Gm-Gg: ASbGncsFrNRbz+bDQ7G+nyKPprIj044Xbs49pF8+V51c1DBh3+nB0fLAXJoUVRop7p+
	6rnYm8ndPy7Is3CQpdGiV9buppmBAnbDhqvxwyx0MtPmb+HWNNo3ckL7U9djdkix7JjVO1ekymT
	pUoUPvtx2yiJ35jyEnh8QPS7DamI7THqlDf08a8tR73Eu8uz0WdrjnXyai/R7TLG7jCtVTnVpcy
	eP3EeC4Y9TwSkwKPXz+bneWMrFv/i4YsHoppZNiIpJUGhpsmKK3vjf2a5aEiOq5WbM+j5sYKuJ2
	nX2rtp58Ow==
X-Received: by 2002:a5d:47c4:0:b0:38a:41f8:8abf with SMTP id ffacd0b85a97d-38a87312917mr29715687f8f.31.1736987462868;
        Wed, 15 Jan 2025 16:31:02 -0800 (PST)
X-Google-Smtp-Source: AGHT+IGI25CYAp0DOP1lGDiOZnqod72mGXaMlzxXJ3kGu+uxJ0OcAaO0K1M4+c8psoSBBjVcUpEmRA==
X-Received: by 2002:a5d:47c4:0:b0:38a:41f8:8abf with SMTP id ffacd0b85a97d-38a87312917mr29715656f8f.31.1736987462151;
        Wed, 15 Jan 2025 16:31:02 -0800 (PST)
Received: from maya.myfinge.rs (ifcgrfdd.trafficplex.cloud. [176.103.220.4])
        by smtp.gmail.com with ESMTPSA id ffacd0b85a97d-38a8e38bf65sm18743664f8f.49.2025.01.15.16.30.59
        (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);
        Wed, 15 Jan 2025 16:31:00 -0800 (PST)
Date: Thu, 16 Jan 2025 01:30:58 +0100
From: Stefano Brivio <sbrivio@redhat.com>
To: Enrique Llorente <ellorent@redhat.com>
Subject: Re: [PATCH v9] dhcp, dhcpv6: Add hostname and client fqdn ops
Message-ID: <20250116013058.0871d04d@elisabeth>
In-Reply-To: <20250115081844.27020-1-ellorent@redhat.com>
References: <20250115081844.27020-1-ellorent@redhat.com>
Organization: Red Hat
X-Mailer: Claws Mail 4.2.0 (GTK 3.24.41; x86_64-pc-linux-gnu)
MIME-Version: 1.0
X-Mimecast-Spam-Score: 0
X-Mimecast-MFC-PROC-ID: BJ9bqtgnAKQhvK5JcpJu77ZlFRmlim4p72szW_SjcEU_1736987464
X-Mimecast-Originator: redhat.com
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Message-ID-Hash: D52VSBRHRDMZCYQZGWPBYQT3NKWMKJZN
X-Message-ID-Hash: D52VSBRHRDMZCYQZGWPBYQT3NKWMKJZN
X-MailFrom: sbrivio@redhat.com
X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; emergency; loop; banned-address; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header
CC: passt-dev@passt.top
X-Mailman-Version: 3.3.8
Precedence: list
List-Id: Development discussion and patches for passt <passt-dev.passt.top>
Archived-At: <https://archives.passt.top/passt-dev/20250116013058.0871d04d@elisabeth/>
Archived-At: <https://passt.top/hyperkitty/list/passt-dev@passt.top/message/D52VSBRHRDMZCYQZGWPBYQT3NKWMKJZN/>
List-Archive: <https://archives.passt.top/passt-dev/>
List-Archive: <https://passt.top/hyperkitty/list/passt-dev@passt.top/>
List-Help: <mailto:passt-dev-request@passt.top?subject=help>
List-Owner: <mailto:passt-dev-owner@passt.top>
List-Post: <mailto:passt-dev@passt.top>
List-Subscribe: <mailto:passt-dev-join@passt.top>
List-Unsubscribe: <mailto:passt-dev-leave@passt.top>

Thanks for sticking with this. The general approach and almost the
whole patch looks good to me now, but there are still a couple of
issues with the options:

On Wed, 15 Jan 2025 09:18:44 +0100
Enrique Llorente <ellorent@redhat.com> wrote:

> Both DHCPv4 and DHCPv6 has the capability to pass the hostname to
> clients, the DHCPv4 uses option 12 (hostname) while the DHCPv6 uses option 39
> (client fqdn), for some virt deployments like kubevirt is expected to
> have the VirtualMachine name as the guest hostname.
> 
> This change add the following arguments:
>  - -H --hostname NAME to configure the hostname DHCPv4 option(12)
>  - --fqdn NAME to configure client fqdn option for both DHCPv4(81) and
>    DHCPv6(39)
> 
> Signed-off-by: Enrique Llorente <ellorent@redhat.com>
> ---
>  conf.c           | 20 +++++++++--
>  dhcp.c           | 60 ++++++++++++++++++++++++++-----
>  dhcpv6.c         | 93 ++++++++++++++++++++++++++++++++++++++++--------
>  passt.1          | 10 ++++++
>  passt.h          |  5 +++
>  pasta.c          | 17 ++++++---
>  test/lib/setup   | 10 +++---
>  test/passt.mbuto |  6 ++--
>  test/passt/dhcp  | 15 +++++++-
>  util.c           | 22 ++++++++++++
>  util.h           |  6 ++++
>  11 files changed, 226 insertions(+), 38 deletions(-)
> 
> diff --git a/conf.c b/conf.c
> index df2b016..0cbd625 100644
> --- a/conf.c
> +++ b/conf.c
> @@ -854,7 +854,9 @@ static void usage(const char *name, FILE *f, int status)
>  		FPRINTF(f, "    default: use addresses from /etc/resolv.conf\n");
>  	FPRINTF(f,
>  		"  -S, --search LIST	Space-separated list, search domains\n"
> -		"    a single, empty option disables the DNS search list\n");
> +		"    a single, empty option disables the DNS search list\n"
> +		"  -H, --hostname NAME 	Hostname to configure client with\n"
> +		"  --fqdn NAME		FQDN to configure client with\n");
>  	if (strstr(name, "pasta"))
>  		FPRINTF(f, "    default: don't use any search list\n");
>  	else
> @@ -1313,6 +1315,7 @@ void conf(struct ctx *c, int argc, char **argv)
>  		{"outbound",	required_argument,	NULL,		'o' },
>  		{"dns",		required_argument,	NULL,		'D' },
>  		{"search",	required_argument,	NULL,		'S' },
> +		{"hostname",	required_argument,	NULL,		'H' },
>  		{"no-tcp",	no_argument,		&c->no_tcp,	1 },
>  		{"no-udp",	no_argument,		&c->no_udp,	1 },
>  		{"no-icmp",	no_argument,		&c->no_icmp,	1 },
> @@ -1357,6 +1360,7 @@ void conf(struct ctx *c, int argc, char **argv)
>  		/* vhost-user backend program convention */
>  		{"print-capabilities", no_argument,	NULL,		26 },
>  		{"socket-path",	required_argument,	NULL,		's' },
> +		{"fqdn",	required_argument,	NULL,		27 },
>  		{ 0 },
>  	};
>  	const char *logname = (c->mode == MODE_PASTA) ? "pasta" : "passt";
> @@ -1379,9 +1383,9 @@ void conf(struct ctx *c, int argc, char **argv)
>  	if (c->mode == MODE_PASTA) {
>  		c->no_dhcp_dns = c->no_dhcp_dns_search = 1;
>  		fwd_default = FWD_AUTO;
> -		optstring = "+dqfel:hF:I:p:P:m:a:n:M:g:i:o:D:S:46t:u:T:U:";
> +		optstring = "+dqfel:hF:I:p:P:m:a:n:M:g:i:o:D:S:H:46t:u:T:U:";
>  	} else {
> -		optstring = "+dqfel:hs:F:p:P:m:a:n:M:g:i:o:D:S:461t:u:";
> +		optstring = "+dqfel:hs:F:p:P:m:a:n:M:g:i:o:D:S:H:461t:u:";
>  	}
>  
>  	c->tcp.fwd_in.mode = c->tcp.fwd_out.mode = FWD_UNSET;
> @@ -1558,6 +1562,11 @@ void conf(struct ctx *c, int argc, char **argv)
>  		case 26:
>  			vu_print_capabilities();
>  			break;
> +		case 27:
> +			if (snprintf_check(c->fqdn, PASST_MAXDNAME,
> +					   "%s", optarg))
> +				die("Invalid FQDN: %s", optarg);
> +			break;
>  		case 'd':
>  			c->debug = 1;
>  			c->quiet = 0;
> @@ -1727,6 +1736,11 @@ void conf(struct ctx *c, int argc, char **argv)
>  
>  			die("Cannot use DNS search domain %s", optarg);
>  			break;
> +		case 'H':
> +			if (snprintf_check(c->hostname, PASST_MAXDNAME,
> +					   "%s", optarg))
> +				die("Invalid hostname: %s", optarg);
> +			break;
>  		case '4':
>  			v4_only = true;
>  			v6_only = false;
> diff --git a/dhcp.c b/dhcp.c
> index d8515aa..ba66f47 100644
> --- a/dhcp.c
> +++ b/dhcp.c
> @@ -63,6 +63,11 @@ static struct opt opts[255];
>  
>  #define OPT_MIN		60 /* RFC 951 */
>  
> +/* Total option size (excluding end option) is 576 (RFC 2131), minus
> + * offset of options (268), minus end option and its length (2).
> + */
> +#define OPT_MAX		306
> +
>  /**
>   * dhcp_init() - Initialise DHCP options
>   */
> @@ -122,7 +127,7 @@ struct msg {
>  	uint8_t sname[64];
>  	uint8_t file[128];
>  	uint32_t magic;
> -	uint8_t o[308];
> +	uint8_t o[OPT_MAX + 2 /* End option and its length */ ];
>  } __attribute__((__packed__));
>  
>  /**
> @@ -130,15 +135,28 @@ struct msg {
>   * @m:		Message to fill
>   * @o:		Option number
>   * @offset:	Current offset within options field, updated on insertion
> + *
> + * Return: false if m has space to write the option, true otherwise
>   */
> -static void fill_one(struct msg *m, int o, int *offset)
> +static bool fill_one(struct msg *m, int o, int *offset)
>  {
> +	size_t slen = opts[o].slen;
> +
> +	/* If we don't have space to write the option, then just skip */
> +	if (*offset + 1 /* length of option */ + slen > OPT_MAX)
> +		return true;
> +
>  	m->o[*offset] = o;
> -	m->o[*offset + 1] = opts[o].slen;
> -	memcpy(&m->o[*offset + 2], opts[o].s, opts[o].slen);
> +	m->o[*offset + 1] = slen;
> +
> +	/* Move to option */
> +	*offset += 2;
> +
> +	memcpy(&m->o[*offset], opts[o].s, slen);
>  
>  	opts[o].sent = 1;
> -	*offset += 2 + opts[o].slen;
> +	*offset += slen;
> +	return false;
>  }
>  
>  /**
> @@ -162,17 +180,20 @@ static int fill(struct msg *m)
>  	 * Put it there explicitly, unless requested via option 55.
>  	 */
>  	if (opts[55].clen > 0 && !memchr(opts[55].c, 53, opts[55].clen))
> -		fill_one(m, 53, &offset);
> +		if (fill_one(m, 53, &offset))
> +			 debug("DHCP: skipping option 55");

We're skipping option 53 in this case, not 55.

>  
>  	for (i = 0; i < opts[55].clen; i++) {
>  		o = opts[55].c[i];
>  		if (opts[o].slen != -1)
> -			fill_one(m, o, &offset);
> +			if (fill_one(m, o, &offset))
> +				debug("DHCP: skipping option %i", o);
>  	}
>  
>  	for (o = 0; o < 255; o++) {
>  		if (opts[o].slen != -1 && !opts[o].sent)
> -			fill_one(m, o, &offset);
> +			if (fill_one(m, o, &offset))
> +				debug("DHCP: skipping option %i", o);
>  	}
>  
>  	m->o[offset++] = 255;
> @@ -398,6 +419,29 @@ int dhcp(const struct ctx *c, const struct pool *p)
>  	if (!opts[6].slen)
>  		opts[6].slen = -1;
>  
> +	opt_len = strlen(c->hostname);
> +	if (opt_len > 0) {
> +		opts[12].slen = opt_len;
> +		memcpy(opts[12].s, &c->hostname, opt_len);
> +	}

So I tried (note the comma):

$ ./passt -d -f --hostname thirtytwocharactersforeachlabel.thirtytwocharactersforeachlabel.thirtytwocharactersforeachlabel.thirtytwocharactersforeachlabel.thirtytwocharactersforeachlabel.thirtytwocharactersforeachlabel.thirtytwocharactersforeachlabel.and_get_to_253_with_this_one, -p fqdn.pcap

$ ./passt -d -f --hostname , -p fqdn.pcap

$ ./passt -d -f --hostname "" -p fqdn.pcap

the hostname and the DHCP reply are fine, but we get an empty FQDN
option in the DHCPv6 reply (only).

Note that RFC 2132 (3.14) says, about option 12:

  See RFC 1035 for character set restrictions.

but I'm not sure if we should validate the user input any further. I
wouldn't. One might legitimately use passt to test the robustness of a
DHCP client.

> +
> +	opt_len = strlen(c->fqdn);
> +	if (opt_len > 0) {
> +		opt_len += 3 /* flags */
> +			+ 2; /* Length byte for first label, and terminator */
> +
> +		if (sizeof(opts[81].s) < opt_len)
> +			debug("DHCP: client FQDN option doesn't fit, skipping");

For this one, I think you actually need to maintain the check in the
caller of encode_domain_name() (that is, here), because of the three
bytes for the flags.

Otherwise, with this:

  $ ./passt -d -f --fqdn
  thirtytwocharactersforeachlabel.thirtytwocharactersforeachlabel.thirtytwocharactersforeachlabel.thirtytwocharactersforeachlabel.thirtytwocharactersforeachlabel.thirtytwocharactersforeachlabel.thirtytwocharactersforeachlabel.and_get_to_253_with_this_one, -p fqdn.pcap

this message is printed, but we end up with an option 81 with length
2, and the FQDN ends up in the padding (try and see capture).

With this:

  $ ./passt -d -f --fqdn
 thirtytwocharactersforeachlabel.thirtytwocharactersforeachlabel.thirtytwocharactersforeachlabel.thirtytwocharactersforeachlabel.thirtytwocharactersforeachlabel.thirtytwocharactersforeachlabel.thirtytwocharactersforeachlabel.and_get_to_252_with_this_one -p fqdn.pcap

length becomes 1, and the FQDN is still in the padding.

With this:

  $ ./passt -d -f --fqdn
 thirtytwocharactersforeachlabel.thirtytwocharactersforeachlabel.thirtytwocharactersforeachlabel.thirtytwocharactersforeachlabel.thirtytwocharactersforeachlabel.thirtytwocharactersforeachlabel.thirtytwocharactersforeachlabel.make_that_251_with_this_one -p fqdn.pcap

length becomes 0, end option is missing, and we have an option 4 (time
server) instead.

With this:

  $ ./passt -d -f --fqdn
 thirtytwocharactersforeachlabel.thirtytwocharactersforeachlabel.thirtytwocharactersforeachlabel.thirtytwocharactersforeachlabel.thirtytwocharactersforeachlabel.thirtytwocharactersforeachlabel.thirtytwocharactersforeachlabel.then_its_250_with_this_one -p fqdn.pcap

things are almost fine. We get one byte of padding, which is probably
not needed, but I didn't look into it.

> +
> +		opts[81].s[0] = 0x4; /* flags (E) */
> +		opts[81].s[1] = 0xff; /* RCODE1 */
> +		opts[81].s[2] = 0xff; /* RCODE2 */
> +
> +		encode_domain_name((char *)opts[81].s + 3, c->fqdn);
> +
> +		opts[81].slen = opt_len;
> +	}
> +
>  	if (!c->no_dhcp_dns_search)
>  		opt_set_dns_search(c, sizeof(m->o));
>  
> diff --git a/dhcpv6.c b/dhcpv6.c
> index 0523bba..ab022d9 100644
> --- a/dhcpv6.c
> +++ b/dhcpv6.c
> @@ -48,6 +48,7 @@ struct opt_hdr {
>  # define  STATUS_NOTONLINK	htons_constant(4)
>  # define OPT_DNS_SERVERS	htons_constant(23)
>  # define OPT_DNS_SEARCH		htons_constant(24)
> +# define OPT_CLIENT_FQDN	htons_constant(39)
>  #define   STR_NOTONLINK		"Prefix not appropriate for link."
>  
>  	uint16_t l;
> @@ -58,6 +59,9 @@ struct opt_hdr {
>  					      sizeof(struct opt_hdr))
>  #define OPT_VSIZE(x)		(sizeof(struct opt_##x) - 		\
>  				 sizeof(struct opt_hdr))
> +#define OPT_MAX_SIZE		IPV6_MIN_MTU - (sizeof(struct ipv6hdr) + \
> +						sizeof(struct udphdr) + \
> +						sizeof(struct msg_hdr))
>  
>  /**
>   * struct opt_client_id - DHCPv6 Client Identifier option
> @@ -163,6 +167,18 @@ struct opt_dns_search {
>  	char list[MAXDNSRCH * NS_MAXDNAME];
>  } __attribute__((packed));
>  
> +/**
> + * struct opt_client_fqdn - Client FQDN option (RFC 4704)
> + * @hdr:		Option header
> + * @flags:		Flags described by RFC 4704
> + * @domain_name:	Client FQDN
> + */
> +struct opt_client_fqdn {
> +	struct opt_hdr hdr;
> +	uint8_t flags;
> +	char domain_name[PASST_MAXDNAME];
> +} __attribute__((packed));
> +
>  /**
>   * struct msg_hdr - DHCPv6 client/server message header
>   * @type:		DHCP message type
> @@ -193,6 +209,7 @@ struct msg_hdr {
>   * @client_id:		Client Identifier, variable length
>   * @dns_servers:	DNS Recursive Name Server, here just for storage size
>   * @dns_search:		Domain Search List, here just for storage size
> + * @client_fqdn:	Client FQDN, variable length
>   */
>  static struct resp_t {
>  	struct msg_hdr hdr;
> @@ -203,6 +220,7 @@ static struct resp_t {
>  	struct opt_client_id client_id;
>  	struct opt_dns_servers dns_servers;
>  	struct opt_dns_search dns_search;
> +	struct opt_client_fqdn client_fqdn;
>  } __attribute__((__packed__)) resp = {
>  	{ 0 },
>  	SERVER_ID,
> @@ -228,6 +246,10 @@ static struct resp_t {
>  	{ { OPT_DNS_SEARCH,	0, },
>  	  { 0 },
>  	},
> +
> +	{ { OPT_CLIENT_FQDN, 0, },
> +	  0, { 0 },
> +	},
>  };
>  
>  static const struct opt_status_code sc_not_on_link = {
> @@ -346,7 +368,6 @@ static size_t dhcpv6_dns_fill(const struct ctx *c, char *buf, int offset)
>  {
>  	struct opt_dns_servers *srv = NULL;
>  	struct opt_dns_search *srch = NULL;
> -	char *p = NULL;
>  	int i;
>  
>  	if (c->no_dhcp_dns)
> @@ -383,34 +404,75 @@ search:
>  		if (!name_len)
>  			continue;
>  
> +		name_len += 2; /* Length byte for first label, and terminator */
> +		if (name_len >
> +		    NS_MAXDNAME + 1 /* Length byte for first label */) {
> +			debug("DHCP: DNS search name '%s' too long, skipping",
> +			      c->dns_search[i].n);
> +			continue;
> +		}

All these issues are pre-existing, but it would be nice to fix at
least the DHCPv6 part of it, now that you're adding checks. With this:

  $ ./passt -d -f -S
 thirtytwocharactersforeachlabel.thirtytwocharactersforeachlabel.thirtytwocharactersforeachlabel.thirtytwocharactersforeachlabel.thirtytwocharactersforeachlabel.thirtytwocharactersforeachlabel.thirtytwocharactersforeachlabel.make_this_overflow_quite_badly -p fqdn.pcap

I made the guest (dhcpcd on Alpine) hang, and with this:

  $ ./passt -d -f -S
 thirtytwocharactersforeachlabel.thirtytwocharactersforeachlabel.thirtytwocharactersforeachlabel.thirtytwocharactersforeachlabel.thirtytwocharactersforeachlabel.thirtytwocharactersforeachlabel.thirtytwocharactersforeachlabel.make_this_overflow_really_badly -p fqdn.pcap

I get a malformed DHCP packet (option 119 with length 1, then the whole
string).

However, I also get a DHCPv6 option 119 with a domain list that's 256
bytes long.

I didn't test much further. The debug message never appears, but I
didn't check why.

> +
>  		if (!srch) {
>  			srch = (struct opt_dns_search *)(buf + offset);
>  			offset += sizeof(struct opt_hdr);
>  			srch->hdr.t = OPT_DNS_SEARCH;
>  			srch->hdr.l = 0;
> -			p = srch->list;
>  		}
>  
> -		*p = '.';
> -		p = stpncpy(p + 1, c->dns_search[i].n, name_len);
> -		p++;
> -		srch->hdr.l += name_len + 2;
> -		offset += name_len + 2;
> +		encode_domain_name(buf + offset, c->dns_search[i].n);
> +
> +		srch->hdr.l += name_len;
> +		offset += name_len;
> +
>  	}
>  
> -	if (srch) {
> -		for (i = 0; i < srch->hdr.l; i++) {
> -			if (srch->list[i] == '.') {
> -				srch->list[i] = strcspn(srch->list + i + 1,
> -							".");
> -			}
> -		}
> +	if (srch)
>  		srch->hdr.l = htons(srch->hdr.l);
> -	}
>  
>  	return offset;
>  }
>  
> +/**
> + * dhcpv6_client_fqdn_fill() - Fill in client FQDN option
> + * @c:		Execution context
> + * @buf:	Response message buffer where options will be appended
> + * @offset:	Offset in message buffer for new options
> + *
> + * Return: updated length of response message buffer.
> + */
> +static size_t dhcpv6_client_fqdn_fill(const struct pool *p, const struct ctx *c,
> +				      char *buf, int offset)
> +
> +{
> +	struct opt_client_fqdn const *req_opt;
> +	struct opt_client_fqdn *o;
> +	size_t opt_len;

dhcpv6_dns_fill() doesn't have this problem, because we have flags telling
us which DNS options we should send, but here you should check that
c->fqdn is actually set, otherwise we'll send an empty option when it's
not specified.

> +	opt_len = strlen(c->fqdn) + 2; /* Length byte for first label, and terminator */

This exceed 80 columns. It's also fine like that, because it's surely
more readable than wrapped, but "Length of first label + terminator"
makes it fit and I think it's equally clear.

> +	if (opt_len > OPT_MAX_SIZE - (offset +
> +				      sizeof(struct opt_hdr) +
> +				      1 /* flags */ )) {
> +		debug("DHCPv6: client FQDN option doesn't fit, skipping");
> +		return offset;
> +	}
> +
> +	o = (struct opt_client_fqdn *)(buf + offset);
> +	encode_domain_name(o->domain_name, c->fqdn);
> +	req_opt = (struct opt_client_fqdn *)dhcpv6_opt(p, &(size_t){ 0 },
> +						       OPT_CLIENT_FQDN);
> +	if (req_opt && req_opt->flags & 0x01 /* S flag */)
> +		o->flags = 0x02 /* O flag */;
> +	else
> +		o->flags = 0x00;
> +
> +	opt_len++;
> +
> +	o->hdr.t = OPT_CLIENT_FQDN;
> +	o->hdr.l = htons(opt_len);
> +
> +	return offset + sizeof(struct opt_hdr) + opt_len;
> +}
> +
>  /**
>   * dhcpv6() - Check if this is a DHCPv6 message, reply as needed
>   * @c:		Execution context
> @@ -544,6 +606,7 @@ int dhcpv6(struct ctx *c, const struct pool *p,
>  	n = offsetof(struct resp_t, client_id) +
>  	    sizeof(struct opt_hdr) + ntohs(client_id->l);
>  	n = dhcpv6_dns_fill(c, (char *)&resp, n);
> +	n = dhcpv6_client_fqdn_fill(p, c, (char *)&resp, n);
>  
>  	resp.hdr.xid = mh->xid;
>  
> diff --git a/passt.1 b/passt.1
> index d9cd33e..7051fc4 100644
> --- a/passt.1
> +++ b/passt.1
> @@ -401,6 +401,16 @@ Enable IPv6-only operation. IPv4 traffic will be ignored.
>  By default, IPv4 operation is enabled as long as at least an IPv4 route and an
>  interface address are configured on a given host interface.
>  
> +.TP
> +.BR \-H ", " \-\-hostname " " \fIname
> +Hostname to configure the client with.
> +Send \fIname\fR as DHCP option 12 (hostname).
> +
> +.TP
> +.BR \-\-fqdn " " \fIname
> +FQDN to configure the client with.
> +Send \fIname\fR as Client FQDN: DHCP option 81 and DHCPv6 option 39.
> +
>  .SS \fBpasst\fR-only options
>  
>  .TP
> diff --git a/passt.h b/passt.h
> index 0dd4efa..f3151f0 100644
> --- a/passt.h
> +++ b/passt.h
> @@ -209,6 +209,8 @@ struct ip6_ctx {
>   * @ifi4:		Template interface for IPv4, -1: none, 0: IPv4 disabled
>   * @ip:			IPv4 configuration
>   * @dns_search:		DNS search list
> + * @hostname:		Guest hostname
> + * @fqdn:		Guest FQDN
>   * @ifi6:		Template interface for IPv6, -1: none, 0: IPv6 disabled
>   * @ip6:		IPv6 configuration
>   * @pasta_ifn:		Name of namespace interface for pasta
> @@ -269,6 +271,9 @@ struct ctx {
>  
>  	struct fqdn dns_search[MAXDNSRCH];
>  
> +	char hostname[PASST_MAXDNAME];
> +	char fqdn[PASST_MAXDNAME];
> +
>  	int ifi6;
>  	struct ip6_ctx ip6;
>  
> diff --git a/pasta.c b/pasta.c
> index ff41c95..922aa10 100644
> --- a/pasta.c
> +++ b/pasta.c
> @@ -169,10 +169,12 @@ void pasta_open_ns(struct ctx *c, const char *netns)
>   * struct pasta_spawn_cmd_arg - Argument for pasta_spawn_cmd()
>   * @exe:	Executable to run
>   * @argv:	Command and arguments to run
> + * @ctx:	Context to read config from
>   */
>  struct pasta_spawn_cmd_arg {
>  	const char *exe;
>  	char *const *argv;
> +	struct ctx *c;
>  };
>  
>  /**
> @@ -186,6 +188,7 @@ static int pasta_spawn_cmd(void *arg)
>  {
>  	char hostname[HOST_NAME_MAX + 1] = HOSTNAME_PREFIX;
>  	const struct pasta_spawn_cmd_arg *a;
> +	size_t conf_hostname_len;
>  	sigset_t set;
>  
>  	/* We run in a detached PID and mount namespace: mount /proc over */
> @@ -195,9 +198,15 @@ static int pasta_spawn_cmd(void *arg)
>  	if (write_file("/proc/sys/net/ipv4/ping_group_range", "0 0"))
>  		warn("Cannot set ping_group_range, ICMP requests might fail");
>  
> -	if (!gethostname(hostname + sizeof(HOSTNAME_PREFIX) - 1,
> -			 HOST_NAME_MAX + 1 - sizeof(HOSTNAME_PREFIX)) ||
> -	    errno == ENAMETOOLONG) {
> +	a = (const struct pasta_spawn_cmd_arg *)arg;
> +
> +	conf_hostname_len = strlen(a->c->hostname);
> +	if (conf_hostname_len > 0) {
> +		if (sethostname(a->c->hostname, conf_hostname_len))
> +			warn("Unable to set configured hostname");
> +	} else if (!gethostname(hostname + sizeof(HOSTNAME_PREFIX) - 1,
> +				HOST_NAME_MAX + 1 - sizeof(HOSTNAME_PREFIX)) ||
> +		   errno == ENAMETOOLONG) {
>  		hostname[HOST_NAME_MAX] = '\0';
>  		if (sethostname(hostname, strlen(hostname)))
>  			warn("Unable to set pasta-prefixed hostname");
> @@ -208,7 +217,6 @@ static int pasta_spawn_cmd(void *arg)
>  	sigaddset(&set, SIGUSR1);
>  	sigwaitinfo(&set, NULL);
>  
> -	a = (const struct pasta_spawn_cmd_arg *)arg;
>  	execvp(a->exe, a->argv);
>  
>  	die_perror("Failed to start command or shell");
> @@ -230,6 +238,7 @@ void pasta_start_ns(struct ctx *c, uid_t uid, gid_t gid,
>  	struct pasta_spawn_cmd_arg arg = {
>  		.exe = argv[0],
>  		.argv = argv,
> +		.c = c,
>  	};
>  	char uidmap[BUFSIZ], gidmap[BUFSIZ];
>  	char *sh_argv[] = { NULL, NULL };
> diff --git a/test/lib/setup b/test/lib/setup
> index 580825f..ee67152 100755
> --- a/test/lib/setup
> +++ b/test/lib/setup
> @@ -49,7 +49,7 @@ setup_passt() {
>  
>  	context_run passt "make clean"
>  	context_run passt "make valgrind"
> -	context_run_bg passt "valgrind --max-stackframe=$((4 * 1024 * 1024)) --trace-children=yes --vgdb=no --error-exitcode=1 --suppressions=test/valgrind.supp ./passt ${__opts} -s ${STATESETUP}/passt.socket -f -t 10001 -u 10001 -P ${STATESETUP}/passt.pid"
> +	context_run_bg passt "valgrind --max-stackframe=$((4 * 1024 * 1024)) --trace-children=yes --vgdb=no --error-exitcode=1 --suppressions=test/valgrind.supp ./passt ${__opts} -s ${STATESETUP}/passt.socket -f -t 10001 -u 10001 -H hostname1 --fqdn fqdn1.passt.test -P ${STATESETUP}/passt.pid"
>  
>  	# pidfile isn't created until passt is listening
>  	wait_for [ -f "${STATESETUP}/passt.pid" ]
> @@ -160,11 +160,11 @@ setup_passt_in_ns() {
>  	if [ ${VALGRIND} -eq 1 ]; then
>  		context_run passt "make clean"
>  		context_run passt "make valgrind"
> -		context_run_bg passt "valgrind --max-stackframe=$((4 * 1024 * 1024)) --trace-children=yes --vgdb=no --error-exitcode=1 --suppressions=test/valgrind.supp ./passt -f ${__opts} -s ${STATESETUP}/passt.socket -t 10001,10011,10021,10031 -u 10001,10011,10021,10031 -P ${STATESETUP}/passt.pid --map-host-loopback ${__map_ns4} --map-host-loopback ${__map_ns6}"
> +		context_run_bg passt "valgrind --max-stackframe=$((4 * 1024 * 1024)) --trace-children=yes --vgdb=no --error-exitcode=1 --suppressions=test/valgrind.supp ./passt -f ${__opts} -s ${STATESETUP}/passt.socket -H hostname1 --fqdn fqdn1.passt.test -t 10001,10011,10021,10031 -u 10001,10011,10021,10031 -P ${STATESETUP}/passt.pid --map-host-loopback ${__map_ns4} --map-host-loopback ${__map_ns6}"
>  	else
>  		context_run passt "make clean"
>  		context_run passt "make"
> -		context_run_bg passt "./passt -f ${__opts} -s ${STATESETUP}/passt.socket -t 10001,10011,10021,10031 -u 10001,10011,10021,10031 -P ${STATESETUP}/passt.pid --map-host-loopback ${__map_ns4} --map-host-loopback ${__map_ns6}"
> +		context_run_bg passt "./passt -f ${__opts} -s ${STATESETUP}/passt.socket -H hostname1 --fqdn fqdn1.passt.test -t 10001,10011,10021,10031 -u 10001,10011,10021,10031 -P ${STATESETUP}/passt.pid --map-host-loopback ${__map_ns4} --map-host-loopback ${__map_ns6}"
>  	fi
>  	wait_for [ -f "${STATESETUP}/passt.pid" ]
>  
> @@ -243,7 +243,7 @@ setup_two_guests() {
>  	[ ${TRACE} -eq 1 ] && __opts="${__opts} --trace"
>  	[ ${VHOST_USER} -eq 1 ] && __opts="${__opts} --vhost-user"
>  
> -	context_run_bg passt_1 "./passt -s ${STATESETUP}/passt_1.socket -P ${STATESETUP}/passt_1.pid -f ${__opts} -t 10001 -u 10001"
> +	context_run_bg passt_1 "./passt -s ${STATESETUP}/passt_1.socket -P ${STATESETUP}/passt_1.pid -f ${__opts} --fqdn fqdn1.passt.test -H hostname1 -t 10001 -u 10001"
>  	wait_for [ -f "${STATESETUP}/passt_1.pid" ]
>  
>  	__opts=
> @@ -252,7 +252,7 @@ setup_two_guests() {
>  	[ ${TRACE} -eq 1 ] && __opts="${__opts} --trace"
>  	[ ${VHOST_USER} -eq 1 ] && __opts="${__opts} --vhost-user"
>  
> -	context_run_bg passt_2 "./passt -s ${STATESETUP}/passt_2.socket -P ${STATESETUP}/passt_2.pid -f ${__opts} -t 10004 -u 10004"
> +	context_run_bg passt_2 "./passt -s ${STATESETUP}/passt_2.socket -P ${STATESETUP}/passt_2.pid -f ${__opts} --hostname hostname2 --fqdn fqdn2 -t 10004 -u 10004"
>  	wait_for [ -f "${STATESETUP}/passt_2.pid" ]
>  
>  	__vmem="$((${MEM_KIB} / 1024 / 4))"
> diff --git a/test/passt.mbuto b/test/passt.mbuto
> index 138d365..1e07693 100755
> --- a/test/passt.mbuto
> +++ b/test/passt.mbuto
> @@ -13,7 +13,7 @@
>  PROGS="${PROGS:-ash,dash,bash ip mount ls insmod mkdir ln cat chmod lsmod
>         modprobe find grep mknod mv rm umount jq iperf3 dhclient hostname
>         sed tr chown sipcalc cut socat dd strace ping tail killall sleep sysctl
> -       nproc tcp_rr tcp_crr udp_rr which tee seq bc sshd ssh-keygen cmp}"
> +       nproc tcp_rr tcp_crr udp_rr which tee seq bc sshd ssh-keygen cmp env}"
>  
>  # OpenSSH 9.8 introduced split binaries, with sshd being the daemon, and
>  # sshd-session the per-session program. We need the latter as well, and the path
> @@ -41,6 +41,7 @@ FIXUP="${FIXUP}"'
>  #!/bin/sh
>  LOG=/var/log/dhclient-script.log
>  echo \${reason} \${interface} >> \$LOG
> +env >> \$LOG
>  set >> \$LOG
>  
>  [ -n "\${new_interface_mtu}" ]       && ip link set dev \${interface} mtu \${new_interface_mtu}
> @@ -54,7 +55,8 @@ set >> \$LOG
>  [ -n "\${new_ip6_address}" ]         && ip addr add \${new_ip6_address}/\${new_ip6_prefixlen} dev \${interface}
>  [ -n "\${new_dhcp6_name_servers}" ]  && for d in \${new_dhcp6_name_servers}; do echo "nameserver \${d}%\${interface}" >> /etc/resolv.conf; done
>  [ -n "\${new_dhcp6_domain_search}" ] && (printf "search"; for d in \${new_dhcp6_domain_search}; do printf " %s" "\${d}"; done; printf "\n") >> /etc/resolv.conf
> -[ -n "\${new_host_name}" ]           && hostname "\${new_host_name}"
> +[ -n "\${new_host_name}" ]           && echo "\${new_host_name}" > /tmp/new_host_name
> +[ -n "\${new_fqdn_fqdn}" ]           && echo "\${new_fqdn_fqdn}" > /tmp/new_fqdn_fqdn
>  exit 0
>  EOF
>  	chmod 755 /sbin/dhclient-script
> diff --git a/test/passt/dhcp b/test/passt/dhcp
> index 9925ab9..145f1ba 100644
> --- a/test/passt/dhcp
> +++ b/test/passt/dhcp
> @@ -11,7 +11,7 @@
>  # Copyright (c) 2021 Red Hat GmbH
>  # Author: Stefano Brivio <sbrivio@redhat.com>
>  
> -gtools	ip jq dhclient sed tr
> +gtools	ip jq dhclient sed tr hostname
>  htools	ip jq sed tr head
>  
>  test	Interface name
> @@ -47,7 +47,16 @@ gout	SEARCH sed 's/\. / /g' /etc/resolv.conf | sed 's/\.$//g' | sed -n 's/^searc
>  hout	HOST_SEARCH sed 's/\. / /g' /etc/resolv.conf | sed 's/\.$//g' | sed -n 's/^search \(.*\)/\1/p' | tr ' \n' ',' | sed 's/,$//;s/$/\n/'
>  check	[ "__SEARCH__" = "__HOST_SEARCH__" ]
>  
> +test	DHCP: Hostname
> +gout	NEW_HOST_NAME cat /tmp/new_host_name
> +check	[ "__NEW_HOST_NAME__" = "hostname1" ]
> +
> +test	DHCP: Client FQDN
> +gout	NEW_FQDN_FQDN cat /tmp/new_fqdn_fqdn
> +check	[ "__NEW_FQDN_FQDN__" = "fqdn1.passt.test" ]
> +
>  test	DHCPv6: address
> +guest	rm /tmp/new_fqdn_fqdn
>  guest	/sbin/dhclient -6 __IFNAME__
>  # Wait for DAD to complete
>  guest	while ip -j -6 addr show tentative | jq -e '.[].addr_info'; do sleep 0.1; done
> @@ -70,3 +79,7 @@ test	DHCPv6: search list
>  gout	SEARCH6 sed 's/\. / /g' /etc/resolv.conf | sed 's/\.$//g' | sed -n 's/^search \(.*\)/\1/p' | tr ' \n' ',' | sed 's/,$//;s/$/\n/'
>  hout	HOST_SEARCH6 sed 's/\. / /g' /etc/resolv.conf | sed 's/\.$//g' | sed -n 's/^search \(.*\)/\1/p' | tr ' \n' ',' | sed 's/,$//;s/$/\n/'
>  check	[ "__SEARCH6__" = "__HOST_SEARCH6__" ]
> +
> +test	DHCPv6: Hostname
> +gout	NEW_FQDN_FQDN cat /tmp/new_fqdn_fqdn
> +check	[ "__NEW_FQDN_FQDN__" = "fqdn1.passt.test" ]
> diff --git a/util.c b/util.c
> index 11973c4..575a879 100644
> --- a/util.c
> +++ b/util.c
> @@ -837,3 +837,25 @@ void raw_random(void *buf, size_t buflen)
>  	if (random_read < buflen)
>  		die("Unexpected EOF on random data source");
>  }
> +/**
> + * encode_domain_name() - Encode domain name according to RFC 1035, section 3.1
> + * @buf:		Buffer to fill in with encoded domain name
> + * @domain_name:	Input domain name string with terminator
> + *
> + * The buffer's 'buf' size has to be >= strlen(domain_name) + 2
> + */
> +void encode_domain_name(char *buf, const char *domain_name)
> +{
> +	size_t i;
> +	char *p;
> +
> +	buf[0] = strcspn(domain_name, ".");
> +	p = buf + 1;
> +	for (i = 0; domain_name[i]; i++) {
> +		if (domain_name[i] == '.')
> +			p[i] = strcspn(domain_name + i + 1, ".");
> +		else
> +			p[i] = domain_name[i];
> +	}
> +	p[i] = 0L;
> +}
> diff --git a/util.h b/util.h
> index 3fa1d12..0744276 100644
> --- a/util.h
> +++ b/util.h
> @@ -40,6 +40,9 @@
>  #ifndef IP_MAX_MTU
>  #define IP_MAX_MTU			USHRT_MAX
>  #endif
> +#ifndef IPV6_MIN_MTU
> +#define IPV6_MIN_MTU			1280
> +#endif
>  
>  #ifndef MIN
>  #define MIN(x, y)		(((x) < (y)) ? (x) : (y))
> @@ -346,4 +349,7 @@ static inline int wrap_accept4(int sockfd, struct sockaddr *addr,
>  #define accept4(s, addr, addrlen, flags) \
>  	wrap_accept4((s), (addr), (addrlen), (flags))
>  
> +#define PASST_MAXDNAME 254 /* 253 (RFC 1035) + 1 (the terminator) */
> +void encode_domain_name(char *buf, const char *domain_name);
> +
>  #endif /* UTIL_H */

The rest looks good to me.

-- 
Stefano