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

3 Feb 2025

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 PM 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 fields
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, 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

    