[PATCH] dhcp: Add option code byte in calculation for OPT_MAX boundary check

[PATCH] dhcp: Add option code byte in calculation for OPT_MAX boundary check

[Stefano Brivio]

Otherwise we'll limit messages to 577 bytes, instead of 576 bytes as
intended:

  $ fqdn="thirtytwocharactersforeachlabel.thirtytwocharactersforeachlabel.thirtytwocharactersforeachlabel.thirtytwocharactersforeachlabel.thirtytwocharactersforeachlabel.thirtytwocharactersforeachlabel.thirtytwocharactersforeachlabel.then_make_it_251_with_this"
  $ hostname="__eighteen_bytes__"
  $ ./pasta --fqdn ${fqdn} -H ${hostname} -p dhcp.pcap -- /sbin/dhclient -4
  Saving packet capture to dhcp.pcap
  $ tshark -r dhcp.pcap -V -Y 'dhcp.option.value == 5' | grep "Total Length"
      Total Length: 577

This was hidden by the issue fixed by commit bcc4908c2b4a ("dhcp
Remove option 255 length byte") until now.

Fixes: 31e8109a86ee ("dhcp, dhcpv6: Add hostname and client fqdn ops")
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
---
 dhcp.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/dhcp.c b/dhcp.c
index 4a209f1..b7d5ea3 100644
--- a/dhcp.c
+++ b/dhcp.c
@@ -143,7 +143,7 @@ static bool fill_one(struct msg *m, int o, int *offset)
 	size_t slen = opts[o].slen;
 
 	/* If we don't have space to write the option, then just skip */
-	if (*offset + 1 /* length of option */ + slen > OPT_MAX)
+	if (*offset + 2 /* code and length of option */ + slen > OPT_MAX)
 		return true;
 
 	m->o[*offset] = o;
-- 
@@ -143,7 +143,7 @@ static bool fill_one(struct msg *m, int o, int *offset)
 	size_t slen = opts[o].slen;
 
 	/* If we don't have space to write the option, then just skip */
-	if (*offset + 1 /* length of option */ + slen > OPT_MAX)
+	if (*offset + 2 /* code and length of option */ + slen > OPT_MAX)
 		return true;
 
 	m->o[*offset] = o;
-- 
2.43.0

Re: [PATCH] dhcp: Add option code byte in calculation for OPT_MAX boundary check

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 1735 bytes --]

On Tue, Feb 18, 2025 at 09:52:31AM +0100, Stefano Brivio wrote:
> Otherwise we'll limit messages to 577 bytes, instead of 576 bytes as
> intended:
> 
>   $ fqdn="thirtytwocharactersforeachlabel.thirtytwocharactersforeachlabel.thirtytwocharactersforeachlabel.thirtytwocharactersforeachlabel.thirtytwocharactersforeachlabel.thirtytwocharactersforeachlabel.thirtytwocharactersforeachlabel.then_make_it_251_with_this"
>   $ hostname="__eighteen_bytes__"
>   $ ./pasta --fqdn ${fqdn} -H ${hostname} -p dhcp.pcap -- /sbin/dhclient -4
>   Saving packet capture to dhcp.pcap
>   $ tshark -r dhcp.pcap -V -Y 'dhcp.option.value == 5' | grep "Total Length"
>       Total Length: 577
> 
> This was hidden by the issue fixed by commit bcc4908c2b4a ("dhcp
> Remove option 255 length byte") until now.
> 
> Fixes: 31e8109a86ee ("dhcp, dhcpv6: Add hostname and client fqdn ops")
> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>

Reviewed-by: David Gibson <david@gibson.dropbear.id.au>

> ---
>  dhcp.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/dhcp.c b/dhcp.c
> index 4a209f1..b7d5ea3 100644
> --- a/dhcp.c
> +++ b/dhcp.c
> @@ -143,7 +143,7 @@ static bool fill_one(struct msg *m, int o, int *offset)
>  	size_t slen = opts[o].slen;
>  
>  	/* If we don't have space to write the option, then just skip */
> -	if (*offset + 1 /* length of option */ + slen > OPT_MAX)
> +	if (*offset + 2 /* code and length of option */ + slen > OPT_MAX)
>  		return true;
>  
>  	m->o[*offset] = o;

-- 
David Gibson (he or they)	| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you, not the other way
				| around.
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] dhcp: Add option code byte in calculation for OPT_MAX boundary check

[Enrique Llorente Pastora]

On Wed, Feb 19, 2025 at 12:21 AM David Gibson
<david@gibson.dropbear.id.au> wrote:
>
> On Tue, Feb 18, 2025 at 09:52:31AM +0100, Stefano Brivio wrote:
> > Otherwise we'll limit messages to 577 bytes, instead of 576 bytes as
> > intended:
> >
> >   $ fqdn="thirtytwocharactersforeachlabel.thirtytwocharactersforeachlabel.thirtytwocharactersforeachlabel.thirtytwocharactersforeachlabel.thirtytwocharactersforeachlabel.thirtytwocharactersforeachlabel.thirtytwocharactersforeachlabel.then_make_it_251_with_this"
> >   $ hostname="__eighteen_bytes__"
> >   $ ./pasta --fqdn ${fqdn} -H ${hostname} -p dhcp.pcap -- /sbin/dhclient -4
> >   Saving packet capture to dhcp.pcap
> >   $ tshark -r dhcp.pcap -V -Y 'dhcp.option.value == 5' | grep "Total Length"
> >       Total Length: 577
> >
> > This was hidden by the issue fixed by commit bcc4908c2b4a ("dhcp
> > Remove option 255 length byte") until now.
> >
> > Fixes: 31e8109a86ee ("dhcp, dhcpv6: Add hostname and client fqdn ops")
> > Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
>
> Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
>
> > ---
> >  dhcp.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/dhcp.c b/dhcp.c
> > index 4a209f1..b7d5ea3 100644
> > --- a/dhcp.c
> > +++ b/dhcp.c
> > @@ -143,7 +143,7 @@ static bool fill_one(struct msg *m, int o, int *offset)
> >       size_t slen = opts[o].slen;
> >
> >       /* If we don't have space to write the option, then just skip */
> > -     if (*offset + 1 /* length of option */ + slen > OPT_MAX)
> > +     if (*offset + 2 /* code and length of option */ + slen > OPT_MAX)
> >               return true;
> >
> >       m->o[*offset] = o;
>
> --
> David Gibson (he or they)       | I'll have my music baroque, and my code
> david AT gibson.dropbear.id.au  | minimalist, thank you, not the other way
>                                 | around.
> http://www.ozlabs.org/~dgibson

I see  "+ 2" better since it is explicit than the alternative of ">=
OPT_MAX", so LGTM.

Reviewed-by: Enrique Llorente <ellorent@redhat.com>

-- 
Quique Llorente

CNV networking Senior Software Engineer

Red Hat EMEA

ellorent@redhat.com

@RedHat   Red Hat  Red Hat