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

On Mon, Feb 3, 2025 at 8:01 PM Stefano Brivio <sbrivio(a)redhat.com> wrote:
...
 
 Sorry for the delay:

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

  On Sat, Feb 1, 2025 at 2:13 PM Stefano Brivio
<sbrivio(a)redhat.com> wrote:
 
 On Fri, 31 Jan 2025 15:53:29 +0100
 Enrique Llorente <ellorent(a)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(a)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(a)redhat.com

@RedHat   Red Hat  Red Hat