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=auDydGm8; 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 0FA085A004E for ; Mon, 03 Feb 2025 10:19:42 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1738574381; 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=0JexMSbbrpLfAzs4LfIJY6ACHdkOu6nff/9gCgl/lg0=; b=auDydGm8QenQBkuh/s8/W4JjXcsiwMop5eMlOG9yQ63WFv6sOb8Wp4G1r4ncOQ/6P8SJlC D1F2NMj4ZZ8dZKeESMPzjtZZwln2DMblql/oNh/Q0JrZaO9AmbjDCeozYiNPt/goFB1BDc QKdWrx218XScOtr6OY4+SOiU3HkiQTg= Received: from mail-ej1-f71.google.com (mail-ej1-f71.google.com [209.85.218.71]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-670-O2nJI_ssMqm4CY0zRDzG6w-1; Mon, 03 Feb 2025 04:19:40 -0500 X-MC-Unique: O2nJI_ssMqm4CY0zRDzG6w-1 X-Mimecast-MFC-AGG-ID: O2nJI_ssMqm4CY0zRDzG6w Received: by mail-ej1-f71.google.com with SMTP id a640c23a62f3a-aa68952272bso7204666b.2 for ; Mon, 03 Feb 2025 01:19:40 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1738574379; x=1739179179; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=0JexMSbbrpLfAzs4LfIJY6ACHdkOu6nff/9gCgl/lg0=; b=dHwWSnMNs6KI5HYbaktCRdSycZPvRv6VI9PNSCEsnxaxgJCCaSE1lD3die3MjD1TRP ccmBVGO+jYzE/vkzNq/qMzbZ16/4BaFP3NM7CVitmNFlq9pT/lM+YsGalgSTvSrsu2wz rch1MXw6lIAhZM/0tRB/D8pPKGA4e5C0k33zrOeASHHcztqncveBGa1bo92VpL044wAv N3Lx3gXStwJEN+T86CfxaJH085SyYlTcCcgZ224aPieos+PmJRjFjaWzmxekU20kMcD4 qI+fjQmxfhW9Up70v6LstE8zRyZxPTJanzUgdGd+IMF8ufoFZS2MQJ99qujAS3Dunsm6 Bk3A== X-Gm-Message-State: AOJu0Yx5sZ4VQoLXroUlSXbGZy5Qx60UvWp3Wmzx2HSX29b6qXlPDdbL KrivbEpvBEC/ERIXf4KikH+kaHzHl7j8uViGg+NLJHMLQdRJT628KW08ICjrtuQKhw1Vce+uVZV 2FnDe4TsKAJYeB2zF8UkIFQK+fM7Gn8krOyyO4vLqV62hZAeAx5IOhXpf0wRL+hHbkZymmu1md5 A3qR2cgr75b7DaVdhwJee775OX X-Gm-Gg: ASbGncvwgz1iFl2rAlje+s50GuTyxFO05hRC4CCRxM3WYDHuofTVgeaLeLBLlv0wJVM 8YoPkoXv6iLwXfpyYzl8clwhdxN3zC1VzPzIwnGeGFlRZ18py930I8NPTK5/yNWnP7hqzQKuyAB sbsNXHtwUpEdMCsaHUmi4S X-Received: by 2002:a17:906:4788:b0:aa6:7a81:3077 with SMTP id a640c23a62f3a-ab6cfdc6163mr1911647266b.54.1738574379124; Mon, 03 Feb 2025 01:19:39 -0800 (PST) X-Google-Smtp-Source: AGHT+IF17kPljA2Q4oQpoeW7uyjmbo2pf324/wrjZFyWuxwA0NeaLqav45yEHfCX+It/OvqeeOyywRU48mGEvqQDV/0= X-Received: by 2002:a17:906:4788:b0:aa6:7a81:3077 with SMTP id a640c23a62f3a-ab6cfdc6163mr1911643866b.54.1738574378620; Mon, 03 Feb 2025 01:19:38 -0800 (PST) MIME-Version: 1.0 References: <20250131145329.1835558-1-ellorent@redhat.com> <20250201141330.59af1324@elisabeth> In-Reply-To: <20250201141330.59af1324@elisabeth> From: Enrique Llorente Pastora Date: Mon, 3 Feb 2025 10:19:27 +0100 X-Gm-Features: AWEUYZkw67p5aysSFcgJL3KAeoLaqhBRRgulqlIJA_wtBmOOISdX89hc81siCT8 Message-ID: Subject: Re: [PATCH] dhcp: Don't re-use request message for reply To: Stefano Brivio X-Mimecast-Spam-Score: 0 X-Mimecast-MFC-PROC-ID: KILurwSfdxhdbhBNnKgQTh-nwalD7RtbzJhoica6f3c_1738574379 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable Message-ID-Hash: 7DAVC76CX64KBSI6NB46WNY3CSLRG3AU X-Message-ID-Hash: 7DAVC76CX64KBSI6NB46WNY3CSLRG3AU X-MailFrom: ellorent@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 Archived-At: Archived-At: List-Archive: List-Archive: List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: On Sat, Feb 1, 2025 at 2:13=E2=80=AFPM Stefano Brivio = wrote: > > On Fri, 31 Jan 2025 15:53:29 +0100 > Enrique Llorente wrote: > > > The logic composing the DHCP reply message is reusing the request > > message to compose the it, this kind be problematic from a security > > Does "be problematic" imply "would be ... once we add longer options"? > > > context and may break the functionality. > > Which one? This is important to know for distribution maintainers and, > ultimately, users. > > As far as I know it's all fine until now, the problem would arise in > your next patch, so perhaps state that. > > The real reason why we need this is that we want to have a given, fixed > size for the option field (308) so that we can easily check we don't > exceed it once we start writing the FQDN in it. > > > This change create a new reply message and fill it in with proper field= s > > from request adding on top the generated opetions. > > s/opetions/options/ > Thinking about it twice, maybe we don't need this change after all, from what I see at code it override the request options with the reply options and then add the 255 finalizer and some padding if is less than 308, meaning that the request options has no effect on reply, isn't that enough ? > > > > Signed-off-by: Enrique Llorente > > --- > > dhcp.c | 55 ++++++++++++++++++++++++++++++++++++++----------------- > > 1 file changed, 38 insertions(+), 17 deletions(-) > > > > diff --git a/dhcp.c b/dhcp.c > > index d8515aa..d8ff330 100644 > > --- a/dhcp.c > > +++ b/dhcp.c > > @@ -142,17 +142,36 @@ static void fill_one(struct msg *m, int o, int *o= ffset) > > } > > > > /** > > - * fill() - Fill options in message > > - * @m: Message to fill > > + * fill() - Fill fields and options in response message > > On one hand, this fits with a function that's called "fill()". On the > other hand, I would have a slight preference to keep this in dhcp() > because dhcp() is a comprehensive summary (albeit a bit long) of what > we're doing. > > Is there a particular reason why you moved non-option field assignments > to here? > > > + * @c: Execution context to copy from > > + * @req: Request message to copy from > > + * @resp: Response Message to write to > > s/Message/message/ > > > * > > * Return: current size of options field > > */ > > -static int fill(struct msg *m) > > +static int fill(const struct ctx *c, struct msg const* req, > > Coding style (assuming you need to change this). > > > + struct msg *resp) > > { > > int i, o, offset =3D 0; > > > > - m->op =3D BOOTREPLY; > > - m->secs =3D 0; > > + resp->op =3D BOOTREPLY; > > + resp->secs =3D 0; > > + resp->hops =3D 0; // We are not a RELAY agent > > Coding style. > > Inconsistency between detail of messages. By this metric, the one above > should also say "We reply FAST" and the one below "This is OLD". > > I don't think these comments really add anything. > > > + memset(&resp->sname, 0, sizeof(resp->sname)); > > + memset(&resp->file, 0, sizeof(resp->file)); > > + resp->yiaddr =3D c->ip4.addr; > > + > > + > > Excess newline. > > > + /* Copy these fields from request */ > > Also rather obvious, plus it would be problematic along with my > suggestion below about having the fields in order. > > > + memcpy(&resp->chaddr, req->chaddr, sizeof(resp->chaddr)); > > + resp->htype =3D req->htype; > > These look like a table, the 'resp' assignments above don't. > > > + resp->hlen =3D req->hlen; > > + resp->xid =3D req->xid; > > + resp->flags =3D req->flags; > > + resp->ciaddr =3D req->ciaddr; > > + resp->siaddr =3D req->siaddr; /* TODO server ip ? */ > > This needs to be 0. The issue is pre-existing, but as you're already > doing this... > > > + resp->giaddr =3D req->giaddr; > > + resp->magic =3D req->magic; > > Could we have *all* those in order? If one is familiar with the > standard (or is reading it), it's easier to follow and find things. > > > for (o =3D 0; o < 255; o++) > > opts[o].sent =3D 0; > > @@ -162,24 +181,24 @@ 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); > > + fill_one(resp, 53, &offset); > > > > for (i =3D 0; i < opts[55].clen; i++) { > > o =3D opts[55].c[i]; > > if (opts[o].slen !=3D -1) > > - fill_one(m, o, &offset); > > + fill_one(resp, o, &offset); > > } > > > > for (o =3D 0; o < 255; o++) { > > if (opts[o].slen !=3D -1 && !opts[o].sent) > > - fill_one(m, o, &offset); > > + fill_one(resp, o, &offset); > > } > > > > - m->o[offset++] =3D 255; > > - m->o[offset++] =3D 0; > > + resp->o[offset++] =3D 255; > > + resp->o[offset++] =3D 0; > > > > if (offset < OPT_MIN) { > > - memset(&m->o[offset], 0, OPT_MIN - offset); > > + memset(&resp->o[offset], 0, OPT_MIN - offset); > > offset =3D OPT_MIN; > > } > > > > @@ -291,8 +310,9 @@ int dhcp(const struct ctx *c, const struct pool *p) > > const struct ethhdr *eh; > > const struct iphdr *iph; > > const struct udphdr *uh; > > + struct msg const *m; > > Look at the line just above. > > > + struct msg resp; > > unsigned int i; > > - struct msg *m; > > > > eh =3D packet_get(p, 0, offset, sizeof(*eh), NULL); > > offset +=3D sizeof(*eh); > > @@ -321,6 +341,7 @@ int dhcp(const struct ctx *c, const struct pool *p) > > m->op !=3D BOOTREQUEST) > > return -1; > > > > + > > Stray change. > > > offset +=3D offsetof(struct msg, o); > > > > for (i =3D 0; i < ARRAY_SIZE(opts); i++) > > @@ -364,7 +385,6 @@ int dhcp(const struct ctx *c, const struct pool *p) > > > > info(" from %s", eth_ntop(m->chaddr, macstr, sizeof(macstr))); > > > > - m->yiaddr =3D c->ip4.addr; > > mask.s_addr =3D htonl(0xffffffff << (32 - c->ip4.prefix_len)); > > memcpy(opts[1].s, &mask, sizeof(mask)); > > memcpy(opts[3].s, &c->ip4.guest_gw, sizeof(c->ip4.guest_gw))= ; > > @@ -399,16 +419,17 @@ int dhcp(const struct ctx *c, const struct pool *= p) > > opts[6].slen =3D -1; > > > > if (!c->no_dhcp_dns_search) > > - opt_set_dns_search(c, sizeof(m->o)); > > + opt_set_dns_search(c, sizeof(resp.o)); > > + > > Stray extra newline. > > > > > - dlen =3D offsetof(struct msg, o) + fill(m); > > + dlen =3D offsetof(struct msg, o) + fill(c, m, &resp); > > > > if (m->flags & FLAG_BROADCAST) > > dst =3D in4addr_broadcast; > > else > > dst =3D c->ip4.addr; > > - > > Stray change. > > > - tap_udp4_send(c, c->ip4.our_tap_addr, 67, dst, 68, m, dlen); > > + tap_udp4_send(c, c->ip4.our_tap_addr, 67, dst, 68, &resp, dlen); > > > > return 1; > > } > > + > > Same here. > > -- > Stefano > -- Quique Llorente CNV networking Senior Software Engineer Red Hat EMEA ellorent@redhat.com @RedHat Red Hat Red Hat