Re: [PATCH] dhcp: Don't re-use request message for reply

[Stefano Brivio <sbrivio@redhat.com>]

Sorry for the delay:

On Mon, 3 Feb 2025 11:26:42 +0100
Enrique Llorente Pastora <ellorent@redhat.com> wrote:

> On Sat, Feb 1, 2025 at 2:13 PM Stefano Brivio <sbrivio@redhat.com> wrote:
> >
> > On Fri, 31 Jan 2025 15:53:29 +0100
> > Enrique Llorente <ellorent@redhat.com> 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 fields
> > > from request adding on top the generated opetions.  
> >
> > s/opetions/options/
> >  
> > >
> > > Signed-off-by: Enrique Llorente <ellorent@redhat.com>
> > > ---
> > >  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 *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 assignments
> > to here?  
> 
> The original fill() function do set some non options fields
> 
> m->op = BOOTREPLY;
> m->secs = 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   = packet_get(p, 0, offset, offsetof(struct msg, o), &opt_len);
>         if (!m                                          ||
>             mlen  != ntohs(uh->len) - sizeof(*uh)       ||
>             mlen  <  offsetof(struct msg, o)            ||
>             m->op != BOOTREQUEST)
>                 return -1;
> 
>         reply.op        = BOOTREPLY;
>         reply.htype     = m->htype;
>         reply.hlen      = m->hlen;
>         reply.hops      = 0;
>         reply.xid       = m->xid;
>         reply.secs      = 0;
>         reply.flags     = m->flags;
>         reply.ciaddr    = m->ciaddr;
>         reply.yiaddr    = c->ip4.addr;
>         reply.siaddr    = 0;
>         reply.giaddr    = 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     = m->magic;

Looks good to me. Maybe use tabs:
	reply.op	= BOOTREPLY;

and this would look even more readable (to me at least, with spaces
too, if it doesn't offend anybody):

	...
	reply.siaddr		= 0;
	reply.giaddr		= 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		= m->magic;

...which is the reason why we don't have/want an automatic formatter. :)

-- 
Stefano