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=hUcSy6xv; dkim-atps=neutral Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by passt.top (Postfix) with ESMTPS id 461045A0275 for ; Tue, 04 Feb 2025 10:27:57 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1738661276; 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=PQhbIWIU1ppw+IC8qk3X8d5FS6Qdv8JFH29fUQuGyDM=; b=hUcSy6xvjAYUrZf+A7usPoXg/kFm7L9jNReObDTIM6/0SKAQU1IjIy2B3uNttCtoVhYNTN ZKdbp96kZ6xavZ0NwmaCB8/6p4Yjc3+c0ZwrPu+CGlpwY9njmGpdV2A8Z/ocJ1FpCTNjZP eBW2BLcPB6KFRqMc9ojwcT9Am21Vrck= Received: from mail-ej1-f72.google.com (mail-ej1-f72.google.com [209.85.218.72]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-93-128fYfpcMwmC0UULhn9U1g-1; Tue, 04 Feb 2025 04:27:54 -0500 X-MC-Unique: 128fYfpcMwmC0UULhn9U1g-1 X-Mimecast-MFC-AGG-ID: 128fYfpcMwmC0UULhn9U1g Received: by mail-ej1-f72.google.com with SMTP id a640c23a62f3a-ab6930f94b7so572011166b.1 for ; Tue, 04 Feb 2025 01:27:54 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1738661273; x=1739266073; 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=PQhbIWIU1ppw+IC8qk3X8d5FS6Qdv8JFH29fUQuGyDM=; b=m/MeZxvheOZyTeU8XgmJjyykBs66m4ru0Wb9oLXkkAazMI9h/K+eMpKIX9q4FBL2m/ YPZGBBqSusHAvhpKNraQ2m2fSTTw5BtJQj4poI25R4kS1ylXGHgCothFHu0QPebSU55E rd/26cqUqJpUfPD/Ynqr0pVxQFscAwegSYsz+GfAoN+ucyoiLJZ01Cr8B3gKrbhhcW3e HCOLRhQdPYimF4fSHmaBFVa1eY9mhq04Spl4KjPzbt+Fwbi8CrmKz65aEXazMr4WT17f N+JHEbpCWjiY7e++7JerMxXAO7hp/eioilTaapp+iG239TefjsBtsgAohrkkR1BaPVKL /qRQ== X-Gm-Message-State: AOJu0Yz7hb+AAKCO/bmv1dmSGVAzg85enFysFvum2tVpJCTD8JGDpYEb h6aYH0HhjbOSJymZsrOzZ9P2M9Dw1M2IFyLcsSfRncBexYOYrlCCropcwvhJM1TnJf6B05vZxfB RRjiKXz18gWP4uaww9AE42UEfK+c8QpIAwm3VUI58O2Wa9NAvXTL9fLiW4wDFhr3N+N9RlIeen/ fyb6kpuJyOlEMc4GNrCUlmzkqEkFc0OtOo14I= X-Gm-Gg: ASbGncvsxaY/+RgHWJu/FtQU08r2FspFYyOgAM9FomnmRqaulciWuZjK/ZaQ5U6qZFD VCELSC4lDfWZOsPDfzlHvPBq2sNLnSujzf3Lc9Cx/kBNbVoL0/YCRgYE52zAWQFQFk5UJZHiO/g M5/piPIIDsn8AUjPPpvWIx X-Received: by 2002:a17:907:1ca7:b0:aaf:123a:e4f0 with SMTP id a640c23a62f3a-ab6cfcb3abamr2960862466b.6.1738661272519; Tue, 04 Feb 2025 01:27:52 -0800 (PST) X-Google-Smtp-Source: AGHT+IFtSx8Twrd33oSQuBs4oqmsW1OiAJ1KML0wptaYYQgmzIT6PDrIVKmGTCCW4NDdicySffsbUR+hL0WchkXJDno= X-Received: by 2002:a17:907:1ca7:b0:aaf:123a:e4f0 with SMTP id a640c23a62f3a-ab6cfcb3abamr2960859566b.6.1738661272053; Tue, 04 Feb 2025 01:27:52 -0800 (PST) MIME-Version: 1.0 References: <20250131145329.1835558-1-ellorent@redhat.com> <20250201141330.59af1324@elisabeth> <20250203200052.7ed1968f@elisabeth> In-Reply-To: <20250203200052.7ed1968f@elisabeth> From: Enrique Llorente Pastora Date: Tue, 4 Feb 2025 10:27:40 +0100 X-Gm-Features: AWEUYZmOODwzi19zV13VV8LUIa6CTCqHuphG7VNx52E1lS54B03gVEcnhMR9UmA 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: Vh7sAIbRwyXzB5kYin0ETwuVsh9Sz9ujJzWiOPvYIJE_1738661274 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable Message-ID-Hash: U732NEEXINQX5RHIMXN564BA5R3PCEHI X-Message-ID-Hash: U732NEEXINQX5RHIMXN564BA5R3PCEHI 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 Mon, Feb 3, 2025 at 8:01=E2=80=AFPM Stefano Brivio = wrote: > > Sorry for the delay: > > On Mon, 3 Feb 2025 11:26:42 +0100 > Enrique Llorente Pastora wrote: > > > 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, fix= ed > > > 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 f= ields > > > > from request adding on top the generated opetions. > > > > > > s/opetions/options/ > > > > > > > > > > > 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, in= t *offset) > > > > } > > > > > > > > /** > > > > - * 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 assignmen= ts > > > to here? > > > > The original fill() function do set some non options fields > > > > m->op =3D BOOTREPLY; > > m->secs =3D 0; > > > > That was my motivation to add the rest of them. > > Hah, funny. Sorry for that, I had no idea. It was actually not intended > on my side. > > > What do you think about doing the opposite and moving op and secs out > > of fill next to the rest of options ? > > Yes, definitely, it makes sense and fits the original intention behind > fill(). > > > The place would be just after parsing request with "packet_get(p, 0, > > offset, offsetof(struct msg, o), &opt_len);" > > > > Something like: > > > > m =3D packet_get(p, 0, offset, offsetof(struct msg, o), &opt_= len); > > if (!m || > > mlen !=3D ntohs(uh->len) - sizeof(*uh) || > > mlen < offsetof(struct msg, o) || > > m->op !=3D BOOTREQUEST) > > return -1; > > > > reply.op =3D BOOTREPLY; > > reply.htype =3D m->htype; > > reply.hlen =3D m->hlen; > > reply.hops =3D 0; > > reply.xid =3D m->xid; > > reply.secs =3D 0; > > reply.flags =3D m->flags; > > reply.ciaddr =3D m->ciaddr; > > reply.yiaddr =3D c->ip4.addr; > > reply.siaddr =3D 0; > > reply.giaddr =3D m->giaddr; > > memcpy(&reply.chaddr, m->chaddr, sizeof(reply.chaddr)); > > memset(&reply.sname, 0, sizeof(reply.sname)); > > memset(&reply.file, 0, sizeof(reply.file)); > > reply.magic =3D m->magic; > > Looks good to me. Maybe use tabs: > reply.op =3D BOOTREPLY; > > and this would look even more readable (to me at least, with spaces > too, if it doesn't offend anybody): > > ... > reply.siaddr =3D 0; > reply.giaddr =3D m->giaddr; > memcpy(&reply.chaddr, m->chaddr, sizeof(reply.chaddr)); > memset(&reply.sname, 0, sizeof(reply.sname)); > memset(&reply.file, 0, sizeof(reply.file)); > reply.magic =3D m->magic; > Ok, I will "tab" so we have first column reply field, second column values and last column sizeofs > ...which is the reason why we don't have/want an automatic formatter. :) > > -- > Stefano > --=20 Quique Llorente CNV networking Senior Software Engineer Red Hat EMEA ellorent@redhat.com @RedHat Red Hat Red Hat