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

[Enrique Llorente Pastora <ellorent@redhat.com>]

On Mon, Feb 3, 2025 at 8:01 PM Stefano Brivio <sbrivio@redhat.com> wrote:
>
> 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;
>

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
>


-- 
Quique Llorente

CNV networking Senior Software Engineer

Red Hat EMEA

ellorent@redhat.com

@RedHat   Red Hat  Red Hat