| Message ID | 1527830811-23372-2-git-send-email-thuth@redhat.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Clean-up and fix-up patches for pxelinux.cfg | expand |
On Fri, 1 Jun 2018 07:26:50 +0200 Thomas Huth <thuth@redhat.com> wrote: > The pxelinux_load_cfg() function always tried to load one byte less than > its parameter said (so that we've got space for a terminating NUL-character > later). This is not very intuitive, let's better ask for one byte less > when we call the function. It's better indeed. > While we're at it, add a sanity check that > the function really did not load more bytes than requested. > No sure for this... see below. > Signed-off-by: Thomas Huth <thuth@redhat.com> > --- > lib/libnet/pxelinux.c | 16 ++++++++++------ > 1 file changed, 10 insertions(+), 6 deletions(-) > > diff --git a/lib/libnet/pxelinux.c b/lib/libnet/pxelinux.c > index 718ceca..eaead48 100644 > --- a/lib/libnet/pxelinux.c > +++ b/lib/libnet/pxelinux.c > @@ -95,7 +95,7 @@ static int pxelinux_load_cfg(filename_ip_t *fn_ip, uint8_t *mac, const char *uui > return -1; > } > strcpy(baseptr, fn_ip->pl_cfgfile); > - rc = pxelinux_tftp_load(fn_ip, cfgbuf, cfgbufsize - 1, retries); > + rc = pxelinux_tftp_load(fn_ip, cfgbuf, cfgbufsize, retries); > if (rc > 0) { > return rc; > } > @@ -104,7 +104,7 @@ static int pxelinux_load_cfg(filename_ip_t *fn_ip, uint8_t *mac, const char *uui > /* Try to load config file with name based on the VM UUID */ > if (uuid) { > strcpy(baseptr, uuid); > - rc = pxelinux_tftp_load(fn_ip, cfgbuf, cfgbufsize - 1, retries); > + rc = pxelinux_tftp_load(fn_ip, cfgbuf, cfgbufsize, retries); > if (rc > 0) { > return rc; > } > @@ -113,7 +113,7 @@ static int pxelinux_load_cfg(filename_ip_t *fn_ip, uint8_t *mac, const char *uui > /* Look for config file with MAC address in its name */ > sprintf(baseptr, "01-%02x-%02x-%02x-%02x-%02x-%02x", > mac[0], mac[1], mac[2], mac[3], mac[4], mac[5]); > - rc = pxelinux_tftp_load(fn_ip, cfgbuf, cfgbufsize - 1, retries); > + rc = pxelinux_tftp_load(fn_ip, cfgbuf, cfgbufsize, retries); > if (rc > 0) { > return rc; > } > @@ -127,7 +127,7 @@ static int pxelinux_load_cfg(filename_ip_t *fn_ip, uint8_t *mac, const char *uui > fn_ip->own_ip & 0xff); > for (idx = 0; idx <= 7; idx++) { > baseptr[8 - idx] = 0; > - rc = pxelinux_tftp_load(fn_ip, cfgbuf, cfgbufsize - 1, > + rc = pxelinux_tftp_load(fn_ip, cfgbuf, cfgbufsize, > retries); > if (rc > 0) { > return rc; > @@ -137,7 +137,7 @@ static int pxelinux_load_cfg(filename_ip_t *fn_ip, uint8_t *mac, const char *uui > > /* Try "default" config file */ > strcpy(baseptr, "default"); > - rc = pxelinux_tftp_load(fn_ip, cfgbuf, cfgbufsize - 1, retries); > + rc = pxelinux_tftp_load(fn_ip, cfgbuf, cfgbufsize, retries); > > return rc; > } > @@ -240,9 +240,13 @@ int pxelinux_load_parse_cfg(filename_ip_t *fn_ip, uint8_t *mac, const char *uuid > { > int rc; > > - rc = pxelinux_load_cfg(fn_ip, mac, uuid, retries, cfgbuf, cfgsize); > + rc = pxelinux_load_cfg(fn_ip, mac, uuid, retries, cfgbuf, cfgsize - 1); > if (rc < 0) > return rc; > + if (rc >= cfgsize) { /* This should never happen */ Can this even happen at all ? I see tftp() seem to have some code that would cause pxelinux_load_cfg() to return -2 if the buffer length is reached... and anyway, if it didn't then rc >= cfgsize would mean something was written past the buffer or am I missing something... how can we cope with that ? > + puts("Downloaded pxelinux.cfg is too big!"); > + return -1; > + } > > return pxelinux_parse_cfg(cfgbuf, rc, entries, max_entries, def_ent); > }
On 01.06.2018 09:24, Greg Kurz wrote: > On Fri, 1 Jun 2018 07:26:50 +0200 > Thomas Huth <thuth@redhat.com> wrote: > >> The pxelinux_load_cfg() function always tried to load one byte less than >> its parameter said (so that we've got space for a terminating NUL-character >> later). This is not very intuitive, let's better ask for one byte less >> when we call the function. > > It's better indeed. > >> While we're at it, add a sanity check that >> the function really did not load more bytes than requested. >> > > No sure for this... see below. > >> Signed-off-by: Thomas Huth <thuth@redhat.com> >> --- >> lib/libnet/pxelinux.c | 16 ++++++++++------ >> 1 file changed, 10 insertions(+), 6 deletions(-) >> >> diff --git a/lib/libnet/pxelinux.c b/lib/libnet/pxelinux.c >> index 718ceca..eaead48 100644 >> --- a/lib/libnet/pxelinux.c >> +++ b/lib/libnet/pxelinux.c >> @@ -95,7 +95,7 @@ static int pxelinux_load_cfg(filename_ip_t *fn_ip, uint8_t *mac, const char *uui >> return -1; >> } >> strcpy(baseptr, fn_ip->pl_cfgfile); >> - rc = pxelinux_tftp_load(fn_ip, cfgbuf, cfgbufsize - 1, retries); >> + rc = pxelinux_tftp_load(fn_ip, cfgbuf, cfgbufsize, retries); >> if (rc > 0) { >> return rc; >> } >> @@ -104,7 +104,7 @@ static int pxelinux_load_cfg(filename_ip_t *fn_ip, uint8_t *mac, const char *uui >> /* Try to load config file with name based on the VM UUID */ >> if (uuid) { >> strcpy(baseptr, uuid); >> - rc = pxelinux_tftp_load(fn_ip, cfgbuf, cfgbufsize - 1, retries); >> + rc = pxelinux_tftp_load(fn_ip, cfgbuf, cfgbufsize, retries); >> if (rc > 0) { >> return rc; >> } >> @@ -113,7 +113,7 @@ static int pxelinux_load_cfg(filename_ip_t *fn_ip, uint8_t *mac, const char *uui >> /* Look for config file with MAC address in its name */ >> sprintf(baseptr, "01-%02x-%02x-%02x-%02x-%02x-%02x", >> mac[0], mac[1], mac[2], mac[3], mac[4], mac[5]); >> - rc = pxelinux_tftp_load(fn_ip, cfgbuf, cfgbufsize - 1, retries); >> + rc = pxelinux_tftp_load(fn_ip, cfgbuf, cfgbufsize, retries); >> if (rc > 0) { >> return rc; >> } >> @@ -127,7 +127,7 @@ static int pxelinux_load_cfg(filename_ip_t *fn_ip, uint8_t *mac, const char *uui >> fn_ip->own_ip & 0xff); >> for (idx = 0; idx <= 7; idx++) { >> baseptr[8 - idx] = 0; >> - rc = pxelinux_tftp_load(fn_ip, cfgbuf, cfgbufsize - 1, >> + rc = pxelinux_tftp_load(fn_ip, cfgbuf, cfgbufsize, >> retries); >> if (rc > 0) { >> return rc; >> @@ -137,7 +137,7 @@ static int pxelinux_load_cfg(filename_ip_t *fn_ip, uint8_t *mac, const char *uui >> >> /* Try "default" config file */ >> strcpy(baseptr, "default"); >> - rc = pxelinux_tftp_load(fn_ip, cfgbuf, cfgbufsize - 1, retries); >> + rc = pxelinux_tftp_load(fn_ip, cfgbuf, cfgbufsize, retries); >> >> return rc; >> } >> @@ -240,9 +240,13 @@ int pxelinux_load_parse_cfg(filename_ip_t *fn_ip, uint8_t *mac, const char *uuid >> { >> int rc; >> >> - rc = pxelinux_load_cfg(fn_ip, mac, uuid, retries, cfgbuf, cfgsize); >> + rc = pxelinux_load_cfg(fn_ip, mac, uuid, retries, cfgbuf, cfgsize - 1); >> if (rc < 0) >> return rc; >> + if (rc >= cfgsize) { /* This should never happen */ > > Can this even happen at all ? I see tftp() seem to have some code that would > cause pxelinux_load_cfg() to return -2 if the buffer length is reached... and > anyway, if it didn't then rc >= cfgsize would mean something was written past > the buffer or am I missing something... how can we cope with that ? Right. As the comment already says, this should (and currently can) never happen. This is just defensive coding, in case somebody ever messes up the TFTP code. In "normal" Linux userspace code, I'd put an assert() here instead - but we don't have support for assert() in SLOF, so I used this if-statement instead. Thomas
On Fri, 1 Jun 2018 09:35:38 +0200 Thomas Huth <thuth@redhat.com> wrote: > On 01.06.2018 09:24, Greg Kurz wrote: > > On Fri, 1 Jun 2018 07:26:50 +0200 > > Thomas Huth <thuth@redhat.com> wrote: > > > >> The pxelinux_load_cfg() function always tried to load one byte less than > >> its parameter said (so that we've got space for a terminating NUL-character > >> later). This is not very intuitive, let's better ask for one byte less > >> when we call the function. > > > > It's better indeed. > > > >> While we're at it, add a sanity check that > >> the function really did not load more bytes than requested. > >> > > > > No sure for this... see below. > > > >> Signed-off-by: Thomas Huth <thuth@redhat.com> > >> --- > >> lib/libnet/pxelinux.c | 16 ++++++++++------ > >> 1 file changed, 10 insertions(+), 6 deletions(-) > >> > >> diff --git a/lib/libnet/pxelinux.c b/lib/libnet/pxelinux.c > >> index 718ceca..eaead48 100644 > >> --- a/lib/libnet/pxelinux.c > >> +++ b/lib/libnet/pxelinux.c > >> @@ -95,7 +95,7 @@ static int pxelinux_load_cfg(filename_ip_t *fn_ip, uint8_t *mac, const char *uui > >> return -1; > >> } > >> strcpy(baseptr, fn_ip->pl_cfgfile); > >> - rc = pxelinux_tftp_load(fn_ip, cfgbuf, cfgbufsize - 1, retries); > >> + rc = pxelinux_tftp_load(fn_ip, cfgbuf, cfgbufsize, retries); > >> if (rc > 0) { > >> return rc; > >> } > >> @@ -104,7 +104,7 @@ static int pxelinux_load_cfg(filename_ip_t *fn_ip, uint8_t *mac, const char *uui > >> /* Try to load config file with name based on the VM UUID */ > >> if (uuid) { > >> strcpy(baseptr, uuid); > >> - rc = pxelinux_tftp_load(fn_ip, cfgbuf, cfgbufsize - 1, retries); > >> + rc = pxelinux_tftp_load(fn_ip, cfgbuf, cfgbufsize, retries); > >> if (rc > 0) { > >> return rc; > >> } > >> @@ -113,7 +113,7 @@ static int pxelinux_load_cfg(filename_ip_t *fn_ip, uint8_t *mac, const char *uui > >> /* Look for config file with MAC address in its name */ > >> sprintf(baseptr, "01-%02x-%02x-%02x-%02x-%02x-%02x", > >> mac[0], mac[1], mac[2], mac[3], mac[4], mac[5]); > >> - rc = pxelinux_tftp_load(fn_ip, cfgbuf, cfgbufsize - 1, retries); > >> + rc = pxelinux_tftp_load(fn_ip, cfgbuf, cfgbufsize, retries); > >> if (rc > 0) { > >> return rc; > >> } > >> @@ -127,7 +127,7 @@ static int pxelinux_load_cfg(filename_ip_t *fn_ip, uint8_t *mac, const char *uui > >> fn_ip->own_ip & 0xff); > >> for (idx = 0; idx <= 7; idx++) { > >> baseptr[8 - idx] = 0; > >> - rc = pxelinux_tftp_load(fn_ip, cfgbuf, cfgbufsize - 1, > >> + rc = pxelinux_tftp_load(fn_ip, cfgbuf, cfgbufsize, > >> retries); > >> if (rc > 0) { > >> return rc; > >> @@ -137,7 +137,7 @@ static int pxelinux_load_cfg(filename_ip_t *fn_ip, uint8_t *mac, const char *uui > >> > >> /* Try "default" config file */ > >> strcpy(baseptr, "default"); > >> - rc = pxelinux_tftp_load(fn_ip, cfgbuf, cfgbufsize - 1, retries); > >> + rc = pxelinux_tftp_load(fn_ip, cfgbuf, cfgbufsize, retries); > >> > >> return rc; > >> } > >> @@ -240,9 +240,13 @@ int pxelinux_load_parse_cfg(filename_ip_t *fn_ip, uint8_t *mac, const char *uuid > >> { > >> int rc; > >> > >> - rc = pxelinux_load_cfg(fn_ip, mac, uuid, retries, cfgbuf, cfgsize); > >> + rc = pxelinux_load_cfg(fn_ip, mac, uuid, retries, cfgbuf, cfgsize - 1); > >> if (rc < 0) > >> return rc; > >> + if (rc >= cfgsize) { /* This should never happen */ > > > > Can this even happen at all ? I see tftp() seem to have some code that would > > cause pxelinux_load_cfg() to return -2 if the buffer length is reached... and > > anyway, if it didn't then rc >= cfgsize would mean something was written past > > the buffer or am I missing something... how can we cope with that ? > > Right. As the comment already says, this should (and currently can) > never happen. This is just defensive coding, in case somebody ever > messes up the TFTP code. In "normal" Linux userspace code, I'd put an > assert() here instead - but we don't have support for assert() in SLOF, > so I used this if-statement instead. What could be an acceptable assert() in SLOF ? Print out a message and do while (1) so that *someone* can try to debug ? > > Thomas
diff --git a/lib/libnet/pxelinux.c b/lib/libnet/pxelinux.c index 718ceca..eaead48 100644 --- a/lib/libnet/pxelinux.c +++ b/lib/libnet/pxelinux.c @@ -95,7 +95,7 @@ static int pxelinux_load_cfg(filename_ip_t *fn_ip, uint8_t *mac, const char *uui return -1; } strcpy(baseptr, fn_ip->pl_cfgfile); - rc = pxelinux_tftp_load(fn_ip, cfgbuf, cfgbufsize - 1, retries); + rc = pxelinux_tftp_load(fn_ip, cfgbuf, cfgbufsize, retries); if (rc > 0) { return rc; } @@ -104,7 +104,7 @@ static int pxelinux_load_cfg(filename_ip_t *fn_ip, uint8_t *mac, const char *uui /* Try to load config file with name based on the VM UUID */ if (uuid) { strcpy(baseptr, uuid); - rc = pxelinux_tftp_load(fn_ip, cfgbuf, cfgbufsize - 1, retries); + rc = pxelinux_tftp_load(fn_ip, cfgbuf, cfgbufsize, retries); if (rc > 0) { return rc; } @@ -113,7 +113,7 @@ static int pxelinux_load_cfg(filename_ip_t *fn_ip, uint8_t *mac, const char *uui /* Look for config file with MAC address in its name */ sprintf(baseptr, "01-%02x-%02x-%02x-%02x-%02x-%02x", mac[0], mac[1], mac[2], mac[3], mac[4], mac[5]); - rc = pxelinux_tftp_load(fn_ip, cfgbuf, cfgbufsize - 1, retries); + rc = pxelinux_tftp_load(fn_ip, cfgbuf, cfgbufsize, retries); if (rc > 0) { return rc; } @@ -127,7 +127,7 @@ static int pxelinux_load_cfg(filename_ip_t *fn_ip, uint8_t *mac, const char *uui fn_ip->own_ip & 0xff); for (idx = 0; idx <= 7; idx++) { baseptr[8 - idx] = 0; - rc = pxelinux_tftp_load(fn_ip, cfgbuf, cfgbufsize - 1, + rc = pxelinux_tftp_load(fn_ip, cfgbuf, cfgbufsize, retries); if (rc > 0) { return rc; @@ -137,7 +137,7 @@ static int pxelinux_load_cfg(filename_ip_t *fn_ip, uint8_t *mac, const char *uui /* Try "default" config file */ strcpy(baseptr, "default"); - rc = pxelinux_tftp_load(fn_ip, cfgbuf, cfgbufsize - 1, retries); + rc = pxelinux_tftp_load(fn_ip, cfgbuf, cfgbufsize, retries); return rc; } @@ -240,9 +240,13 @@ int pxelinux_load_parse_cfg(filename_ip_t *fn_ip, uint8_t *mac, const char *uuid { int rc; - rc = pxelinux_load_cfg(fn_ip, mac, uuid, retries, cfgbuf, cfgsize); + rc = pxelinux_load_cfg(fn_ip, mac, uuid, retries, cfgbuf, cfgsize - 1); if (rc < 0) return rc; + if (rc >= cfgsize) { /* This should never happen */ + puts("Downloaded pxelinux.cfg is too big!"); + return -1; + } return pxelinux_parse_cfg(cfgbuf, rc, entries, max_entries, def_ent); }
The pxelinux_load_cfg() function always tried to load one byte less than its parameter said (so that we've got space for a terminating NUL-character later). This is not very intuitive, let's better ask for one byte less when we call the function. While we're at it, add a sanity check that the function really did not load more bytes than requested. Signed-off-by: Thomas Huth <thuth@redhat.com> --- lib/libnet/pxelinux.c | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-)