From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: passt.top; dmarc=none (p=none dis=none) header.from=gibson.dropbear.id.au Authentication-Results: passt.top; dkim=pass (2048-bit key; secure) header.d=gibson.dropbear.id.au header.i=@gibson.dropbear.id.au header.a=rsa-sha256 header.s=202602 header.b=m31EJteS; dkim-atps=neutral Received: from mail.ozlabs.org (gandalf.ozlabs.org [150.107.74.76]) by passt.top (Postfix) with ESMTPS id AED455A026E for ; Tue, 19 May 2026 08:12:05 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gibson.dropbear.id.au; s=202602; t=1779171122; bh=dxg1u2H6/1Pe0w+MZZ7GLjrdtuSqTvvpdUbUKZ/dr1E=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=m31EJteS8yfnia66T5l7X5tAqDYxN1mNqNN2MJDKpQVLlV7apd878MQg7Z9S9lSWO +dm4nrrTbndbor02XIFrY9z0jOK/w1Tyr/oPYbFuztbjhcBm/mQv71ISHtpZU2LKNd 5NGA7kLXg20+ByPdbn4yQLaR6TbmLrUuB/cfWYwlrQKD7/MYekhsmo/0U5iFg6Kzh8 h675HDVUVs/r8ou7EmTIEN2WZrV0F2kHMFUgil0l5CzZ9WRX+zNne1PMrc+FeKFACe gNq0sAV2IADd6D+z3iPXNE4objoQQ+JLptcZbzP10x0BD952h1FF5LtN/KzDgAZmRx 1VEM5fWLzWUsg== Received: by gandalf.ozlabs.org (Postfix, from userid 1007) id 4gKPTG147cz4wQJ; Tue, 19 May 2026 16:12:02 +1000 (AEST) Date: Tue, 19 May 2026 15:59:06 +1000 From: David Gibson To: Anshu Kumari Subject: Re: [PATCH 3/6] dhcp: Add option type table and value parser Message-ID: References: <20260518132002.418296-1-anskuma@redhat.com> <20260518132002.418296-2-anskuma@redhat.com> <20260518132002.418296-3-anskuma@redhat.com> <20260518132002.418296-4-anskuma@redhat.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha512; protocol="application/pgp-signature"; boundary="vqjF9yCWVVNi/o2k" Content-Disposition: inline In-Reply-To: <20260518132002.418296-4-anskuma@redhat.com> Message-ID-Hash: M2WCF3BW2NOTB6JXF575FFHALJVK2WVN X-Message-ID-Hash: M2WCF3BW2NOTB6JXF575FFHALJVK2WVN X-MailFrom: dgibson@gandalf.ozlabs.org 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: sbrivio@redhat.com, passt-dev@passt.top, lvivier@redhat.com, jmaloy@redhat.com X-Mailman-Version: 3.3.8 Precedence: list List-Id: Development discussion and patches for passt Archived-At: Archived-At: List-Archive: List-Archive: List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: --vqjF9yCWVVNi/o2k Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Mon, May 18, 2026 at 06:49:59PM +0530, Anshu Kumari wrote: > Add an RFC 2132 type lookup table mapping DHCP option codes to their > expected value formats, and a dhcp_opt_parse() function that converts > CLI string values into their binary wire representation. >=20 > Wire dhcp_opt_parse() into the --dhcp-opt handler so that values are > validated and encoded at configuration time. >=20 > Link: https://bugs.passt.top/show_bug.cgi?id=3D192 > Signed-off-by: Anshu Kumari > --- > conf.c | 9 +++ > dhcp.c | 227 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++ > dhcp.h | 15 ++++ > 3 files changed, 251 insertions(+) >=20 > diff --git a/conf.c b/conf.c > index 61a393f..3ec10ac 100644 > --- a/conf.c > +++ b/conf.c > @@ -1485,6 +1485,7 @@ void conf(struct ctx *c, int argc, char **argv) > unsigned long code; > const char *comma; > char *end; > + int len; > =20 > comma =3D strchr(optarg, ','); > if (!comma) > @@ -1499,7 +1500,15 @@ void conf(struct ctx *c, int argc, char **argv) > die("Too many --dhcp-opt entries (max %d)", > MAX_CUSTOM_DHCP_OPTS); > =20 > + len =3D dhcp_opt_parse(code, comma + 1, > + c->custom_opts[c->custom_opts_count].val, > + sizeof(c->custom_opts[0].val)); > + if (len < 0) > + die("Invalid value for DHCP option %lu: %s", > + code, comma + 1); > + > c->custom_opts[c->custom_opts_count].code =3D code; > + c->custom_opts[c->custom_opts_count].len =3D len; I don't love the fact that .val and .str permanently store redundant information. Initially I was going to suggest that you delay dhcp_opt_parse() until you're actually processing a DHCP message. However, that's also not great at that point it awkward to handle parse errors. Not sure how best to tackle this for now. > if (snprintf_check(c->custom_opts[c->custom_opts_count].str, > sizeof(c->custom_opts[0].str), > "%s", comma + 1)) > diff --git a/dhcp.c b/dhcp.c > index 1ff8cba..9220516 100644 > --- a/dhcp.c > +++ b/dhcp.c > @@ -33,6 +33,233 @@ > #include "log.h" > #include "dhcp.h" > =20 > +/** > + * struct dhcp_opt_type_entry - Maps option code to RFC 2132 value type > + * @code: DHCP option code > + * @type: Expected value format > + */ > +static const struct dhcp_opt_type_entry { > + uint8_t code; > + enum dhcp_opt_type type; > +} dhcp_opt_types[] =3D { > + { 1, DHCP_OPT_IPV4 }, /* Subnet Mask */ > + { 2, DHCP_OPT_UINT32 }, /* Time Offset */ > + { 3, DHCP_OPT_IPV4_LIST }, /* Router */ > + { 4, DHCP_OPT_IPV4_LIST }, /* Time Server */ > + { 5, DHCP_OPT_IPV4_LIST }, /* Name Server */ > + > + { 6, DHCP_OPT_IPV4_LIST }, /* Domain Name Server */ > + { 7, DHCP_OPT_IPV4_LIST }, /* Log Server */ > + { 8, DHCP_OPT_IPV4_LIST }, /* Cookie Server */ > + { 9, DHCP_OPT_IPV4_LIST }, /* LPR Server */ > + { 10, DHCP_OPT_IPV4_LIST }, /* Impress Server */ > + > + { 11, DHCP_OPT_IPV4_LIST }, /* Resource Location Server */ > + { 12, DHCP_OPT_STR }, /* Host Name */ > + { 13, DHCP_OPT_UINT16 }, /* Boot File Size */ > + { 15, DHCP_OPT_STR }, /* Domain Name */ > + { 16, DHCP_OPT_IPV4 }, /* Swap Server */ > + > + { 17, DHCP_OPT_STR }, /* Root Path */ > + { 19, DHCP_OPT_UINT8 }, /* IP Forwarding */ > + { 23, DHCP_OPT_UINT8 }, /* Default IP TTL */ > + { 26, DHCP_OPT_UINT16 }, /* Interface MTU */ > + { 28, DHCP_OPT_IPV4 }, /* Broadcast Address */ > + > + { 33, DHCP_OPT_IPV4_LIST }, /* Static Routes (dest+router pairs) */ > + { 37, DHCP_OPT_UINT8 }, /* TCP Default TTL */ > + { 38, DHCP_OPT_UINT32 }, /* TCP Keepalive Interval */ > + { 40, DHCP_OPT_STR }, /* NIS Domain Name */ > + { 41, DHCP_OPT_IPV4_LIST }, /* NIS Servers */ > + > + { 42, DHCP_OPT_IPV4_LIST }, /* NTP Servers */ > + { 44, DHCP_OPT_IPV4_LIST }, /* NetBIOS Name Server */ > + { 50, DHCP_OPT_IPV4 }, /* Requested IP Address */ > + { 51, DHCP_OPT_UINT32 }, /* IP Address Lease Time */ > + { 53, DHCP_OPT_UINT8 }, /* DHCP Message Type */ > + > + { 54, DHCP_OPT_IPV4 }, /* Server Identifier */ > + { 57, DHCP_OPT_UINT16 }, /* Max DHCP Message Size */ > + { 58, DHCP_OPT_UINT32 }, /* Renewal (T1) Time */ > + { 59, DHCP_OPT_UINT32 }, /* Rebinding (T2) Time */ > + { 60, DHCP_OPT_STR }, /* Vendor Class Identifier */ > + > + { 61, DHCP_OPT_STR }, /* Client Identifier */ > + { 66, DHCP_OPT_STR }, /* TFTP Server Name */ > + { 67, DHCP_OPT_STR }, /* Bootfile Name */ > + { 119, DHCP_OPT_STR }, /* Domain Search List (RFC 3397) */ > + { 121, DHCP_OPT_ROUTES }, /* Classless Static Routes */ > + > + { 252, DHCP_OPT_STR }, /* WPAD URL */ At least initially, I'd suggest we don't permit the user to set options which passt already manages internally (1, 3, 51, 53, 54, 55, and 199 from a quick scan). > +}; > + > +/** > + * dhcp_opt_type_lookup() - Look up the value type for a DHCP option code > + * @code: DHCP option code > + * > + * Return: type from table > + */ > +static enum dhcp_opt_type dhcp_opt_type_lookup(uint8_t code) > +{ > + unsigned int i; > + > + for (i =3D 0; i < ARRAY_SIZE(dhcp_opt_types); i++) { > + if (dhcp_opt_types[i].code =3D=3D code) > + return dhcp_opt_types[i].type; > + } > + > + return DHCP_OPT_NONE; > +} > + > +/** > + * dhcp_opt_parse() - Parse a DHCP option value > + * @code: DHCP option code (determines value type via lookup table) > + * @str: Value string from command line > + * @buf: Output buffer for binary value > + * @buf_len: Size of output buffer > + * > + * Return: number of bytes written to @buf, or -1 on error > + */ > +int dhcp_opt_parse(uint8_t code, const char *str, uint8_t *buf, size_t b= uf_len) > +{ > + enum dhcp_opt_type type =3D dhcp_opt_type_lookup(code); > + > + switch (type) { > + case DHCP_OPT_NONE: { > + die("Unsupported DHCP option: %u," > + " see passt(1) for supported codes", > + code); > + } > + case DHCP_OPT_IPV4: { > + struct in_addr addr; > + > + if (inet_pton(AF_INET, str, &addr) !=3D 1) > + return -1; > + if (buf_len < sizeof(addr)) > + return -1; > + memcpy(buf, &addr, sizeof(addr)); > + return sizeof(addr); > + } > + case DHCP_OPT_IPV4_LIST: { > + char *tok, *saveptr; > + char tmp[1024]; Bare 1024 constant isn't ideal. > + int len =3D 0; > + > + if (snprintf_check(tmp, sizeof(tmp), "%s", str)) > + return -1; > + > + for (tok =3D strtok_r(tmp, " ", &saveptr); tok; > + tok =3D strtok_r(NULL, " ", &saveptr)) { Apparently strtok_r() is specified by POSIX, so it should be ok. Might be worth double checking musl has it, though. " " seems an odd choice of delimiter - it's pretty awkward to use on the command line. I guess "," is also problematic since it can be found within various of the of the option values. > + struct in_addr addr; > + > + if (inet_pton(AF_INET, tok, &addr) !=3D 1) > + return -1; > + if (len + (int)sizeof(addr) > (int)buf_len) > + return -1; > + memcpy(buf + len, &addr, sizeof(addr)); > + len +=3D sizeof(addr); > + } > + return len; > + } > + case DHCP_OPT_UINT8: { > + unsigned long val; > + char *end; > + > + val =3D strtoul(str, &end, 0); > + if (*end || val > 255 || buf_len < 1) > + return -1; > + buf[0] =3D val; > + return 1; > + } > + case DHCP_OPT_UINT16: { > + unsigned long val; > + char *end; > + > + val =3D strtoul(str, &end, 0); > + if (*end || val > 65535 || buf_len < 2) > + return -1; > + buf[0] =3D (val >> 8) & 0xff; > + buf[1] =3D val & 0xff; > + return 2; > + } > + case DHCP_OPT_UINT32: { > + unsigned long val; > + char *end; > + > + val =3D strtoul(str, &end, 0); > + if (*end || buf_len < 4) > + return -1; > + buf[0] =3D (val >> 24) & 0xff; > + buf[1] =3D (val >> 16) & 0xff; > + buf[2] =3D (val >> 8) & 0xff; > + buf[3] =3D val & 0xff; > + return 4; > + } It might be possible to simplify the sections above using a single DHCP_OPT_INTEGER type, plus an extra field giving the integer width. > + case DHCP_OPT_ROUTES: { I'd consider excluding this one for now. For one thing it's rather a lot of code. But more importantly with the multi-address and route monitoring changes we're considering, there's a fairly high chance we might want to manage this one ourselves. > + /* RFC 3442: "CIDR/mask,gateway" entries, space-separated > + * Encodes as: mask-width + significant-octets + router > + * e.g. "192.168.1.0/24,10.0.0.1 0.0.0.0/0,10.0.0.1" > + */ > + char *tok, *saveptr; > + char tmp[1024]; > + int len =3D 0; > + > + if (snprintf_check(tmp, sizeof(tmp), "%s", str)) > + return -1; > + > + for (tok =3D strtok_r(tmp, " ", &saveptr); tok; > + tok =3D strtok_r(NULL, " ", &saveptr)) { > + struct in_addr dest, gw; > + char *slash, *comma; > + unsigned long mask; > + int sig_octets; > + > + slash =3D strchr(tok, '/'); > + if (!slash) > + return -1; > + *slash =3D '\0'; > + > + if (inet_pton(AF_INET, tok, &dest) !=3D 1) > + return -1; > + > + comma =3D strchr(slash + 1, ','); > + if (!comma) > + return -1; > + *comma =3D '\0'; > + > + mask =3D strtoul(slash + 1, NULL, 10); > + if (mask > 32) > + return -1; > + > + if (inet_pton(AF_INET, comma + 1, &gw) !=3D 1) > + return -1; > + > + sig_octets =3D (mask + 7) / 8; > + > + if (len + 1 + sig_octets + 4 > (int)buf_len) > + return -1; > + > + buf[len++] =3D mask; > + memcpy(buf + len, &dest, sig_octets); > + len +=3D sig_octets; > + memcpy(buf + len, &gw, 4); > + len +=3D 4; > + } > + return len; > + } > + case DHCP_OPT_STR: { > + size_t len =3D strlen(str); > + > + if (!len || len >=3D buf_len) > + return -1; > + strncpy((char *)buf, str, buf_len); > + return len; > + } > + } > + > + return -1; > +} > + > /** > * struct opt - DHCP option > * @sent: Convenience flag, set while filling replies > diff --git a/dhcp.h b/dhcp.h > index cd50c99..01b2290 100644 > --- a/dhcp.h > +++ b/dhcp.h > @@ -6,7 +6,22 @@ > #ifndef DHCP_H > #define DHCP_H > =20 > +/** > + * enum dhcp_opt_type - DHCP option value types per RFC 2132 > + */ > +enum dhcp_opt_type { > + DHCP_OPT_NONE, > + DHCP_OPT_STR, > + DHCP_OPT_IPV4, > + DHCP_OPT_IPV4_LIST, > + DHCP_OPT_UINT8, > + DHCP_OPT_UINT16, > + DHCP_OPT_UINT32, > + DHCP_OPT_ROUTES, > +}; > + > int dhcp(const struct ctx *c, struct iov_tail *data); > void dhcp_init(void); > +int dhcp_opt_parse(uint8_t code, const char *str, uint8_t *buf, size_t b= uf_len); > =20 > #endif /* DHCP_H */ > --=20 > 2.54.0 >=20 --=20 David Gibson (he or they) | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you, not the other way | around. http://www.ozlabs.org/~dgibson --vqjF9yCWVVNi/o2k Content-Type: application/pgp-signature; name=signature.asc -----BEGIN PGP SIGNATURE----- iQIzBAEBCgAdFiEEO+dNsU4E3yXUXRK2zQJF27ox2GcFAmoL/CkACgkQzQJF27ox 2GfLxQ//cbzB3RQUHKMZwFpOfEfZ8KWkcGqDjrSsbOAKRj0AGinoCxIEhDjdnEbC bg65s5mHUrgtp5Lip/iU87Ek9hQM8MvwyNCqpdvWU2RjuTj+JbMj5YuElITDLo0K ccVRuVTVESd5I+w6aT5eXHiQTayOSL/acKodsr4gSzsooeJo/1OdLuA67X8OBbl7 l8761zwrv8jC8PNZQNvQkU5EWO0NB4yr9aOFtDsid7wGYxzY2fwVA0EV6P9yE4kC 91yKNaEnbwN1tzBs0KlKi/YbvSg+jBWQxE5pXF8cr2/sqy6/5EEg463L45dGS8mK uZ4mU+yxIv0cced0P2FpkqM+VUGwYUZy2/+n+8r16QEJwU8EQkT8n3HJtIpK8mkG 6DCupO1c0aqUa/Imzsz2j6hQ5JGD0F+o9M/TC8nYJDVPjU8NwGNjv7k5GZK0yweB iXqchhaRJI/kqOZU3VoGQCmbxcaeRNCPWECDXGGKdDYZOE1SggPZ5Vj7fAY9l+EO c0fQjW4hXQ9QfMXyVngqzNObTE2K5l9Ga1tfRenhzzsuwjhJ4bAGZRVp9hKnhUA9 968Y+gu7QSQdH83oYecrSViMMZongzqvjrUdXpWn/97Z528rG40VIGI1WfYBFXcd vuQ5WX/vVkcU8ztZG+eIv+QjbldrEKYPBpe23HJr5LF7O/LqzTg= =gKRE -----END PGP SIGNATURE----- --vqjF9yCWVVNi/o2k--