fix: NULL dereference in bios(4)

classic Classic list List threaded Threaded
7 messages Options
Reply | Threaded
Open this post in threaded view
|

fix: NULL dereference in bios(4)

Jan Klemkow
Hi,

fixstring() can return NULL and it does on one of my machines.
Here is s fix that checks for NULL and return an empty string.

ok?

bye,
Jan

Index: arch/amd64/amd64/bios.c
===================================================================
RCS file: /cvs/src/sys/arch/amd64/amd64/bios.c,v
retrieving revision 1.38
diff -u -p -r1.38 bios.c
--- arch/amd64/amd64/bios.c 15 Jul 2019 00:35:10 -0000 1.38
+++ arch/amd64/amd64/bios.c 19 Jul 2019 08:56:27 -0000
@@ -144,7 +144,9 @@ bios_attach(struct device *parent, struc
     fixstring(scratch));
  if ((smbios_get_string(&bios, sb->release,
     scratch, sizeof(scratch))) != NULL) {
- strlcpy(smbios_bios_date, fixstring(scratch),
+ strlcpy(smbios_bios_date,
+    fixstring(scratch) == NULL ? "" :
+ fixstring(scratch),
     sizeof(smbios_bios_date));
  printf(" date %s", fixstring(scratch));
  }

Reply | Threaded
Open this post in threaded view
|

Re: fix: NULL dereference in bios(4)

Jonathan Gray-11
On Fri, Jul 19, 2019 at 01:07:34PM +0200, Jan Klemkow wrote:
> Hi,
>
> fixstring() can return NULL and it does on one of my machines.
> Here is s fix that checks for NULL and return an empty string.
>
> ok?

If it returns NULL shouldn't the strlcpy and printf both be skipped?

You've also not included the i386 portion of this.

>
> bye,
> Jan
>
> Index: arch/amd64/amd64/bios.c
> ===================================================================
> RCS file: /cvs/src/sys/arch/amd64/amd64/bios.c,v
> retrieving revision 1.38
> diff -u -p -r1.38 bios.c
> --- arch/amd64/amd64/bios.c 15 Jul 2019 00:35:10 -0000 1.38
> +++ arch/amd64/amd64/bios.c 19 Jul 2019 08:56:27 -0000
> @@ -144,7 +144,9 @@ bios_attach(struct device *parent, struc
>      fixstring(scratch));
>   if ((smbios_get_string(&bios, sb->release,
>      scratch, sizeof(scratch))) != NULL) {
> - strlcpy(smbios_bios_date, fixstring(scratch),
> + strlcpy(smbios_bios_date,
> +    fixstring(scratch) == NULL ? "" :
> + fixstring(scratch),
>      sizeof(smbios_bios_date));
>   printf(" date %s", fixstring(scratch));
>   }
>

Reply | Threaded
Open this post in threaded view
|

Re: fix: NULL dereference in bios(4)

Jan Klemkow
On Fri, Jul 19, 2019 at 09:13:38PM +1000, Jonathan Gray wrote:
> On Fri, Jul 19, 2019 at 01:07:34PM +0200, Jan Klemkow wrote:
> > Hi,
> >
> > fixstring() can return NULL and it does on one of my machines.
> > Here is s fix that checks for NULL and return an empty string.
> >
> > ok?
>
> If it returns NULL shouldn't the strlcpy and printf both be skipped?

I was unsure about this.

> You've also not included the i386 portion of this.

I missed that.

The diff below addresses all issues.

ok?

Thanks,
Jan

Index: arch/amd64/amd64/bios.c
===================================================================
RCS file: /cvs/src/sys/arch/amd64/amd64/bios.c,v
retrieving revision 1.38
diff -u -p -r1.38 bios.c
--- arch/amd64/amd64/bios.c 15 Jul 2019 00:35:10 -0000 1.38
+++ arch/amd64/amd64/bios.c 19 Jul 2019 11:45:11 -0000
@@ -144,9 +144,11 @@ bios_attach(struct device *parent, struc
     fixstring(scratch));
  if ((smbios_get_string(&bios, sb->release,
     scratch, sizeof(scratch))) != NULL) {
- strlcpy(smbios_bios_date, fixstring(scratch),
+ char *s = fixstring(scratch) == NULL ? "" :
+    fixstring(scratch);
+ strlcpy(smbios_bios_date, s,
     sizeof(smbios_bios_date));
- printf(" date %s", fixstring(scratch));
+ printf(" date %s", s);
  }
  }
 
Index: arch/i386/i386/bios.c
===================================================================
RCS file: /cvs/src/sys/arch/i386/i386/bios.c,v
retrieving revision 1.121
diff -u -p -r1.121 bios.c
--- arch/i386/i386/bios.c 15 Jul 2019 00:35:10 -0000 1.121
+++ arch/i386/i386/bios.c 19 Jul 2019 11:58:57 -0000
@@ -308,10 +308,11 @@ biosattach(struct device *parent, struct
     fixstring(scratch));
  if ((smbios_get_string(&bios, sb->release,
     scratch, sizeof(scratch))) != NULL) {
- strlcpy(smbios_bios_date,
-    fixstring(scratch),
+ char *s = fixstring(scratch) == NULL ?
+    "" : fixstring(scratch);
+ strlcpy(smbios_bios_date, s,
     sizeof(smbios_bios_date));
- printf(" date %s", fixstring(scratch));
+ printf(" date %s", s);
  }
  }
  smbios_info(sc->sc_dev.dv_xname);

Reply | Threaded
Open this post in threaded view
|

Re: fix: NULL dereference in bios(4)

Jonathan Gray-11
On Fri, Jul 19, 2019 at 02:15:03PM +0200, Jan Klemkow wrote:

> On Fri, Jul 19, 2019 at 09:13:38PM +1000, Jonathan Gray wrote:
> > On Fri, Jul 19, 2019 at 01:07:34PM +0200, Jan Klemkow wrote:
> > > Hi,
> > >
> > > fixstring() can return NULL and it does on one of my machines.
> > > Here is s fix that checks for NULL and return an empty string.
> > >
> > > ok?
> >
> > If it returns NULL shouldn't the strlcpy and printf both be skipped?
>
> I was unsure about this.
>
> > You've also not included the i386 portion of this.
>
> I missed that.
>
> The diff below addresses all issues.
>
> ok?

What argument does fixstring have in this case?

Does you dmesg not include a date in the 'bios0' line?

I was considering making this a pointer/dynamically allocated so it can
be NULL.  If there is no valid date it should probably be NULL.

>
> Thanks,
> Jan
>
> Index: arch/amd64/amd64/bios.c
> ===================================================================
> RCS file: /cvs/src/sys/arch/amd64/amd64/bios.c,v
> retrieving revision 1.38
> diff -u -p -r1.38 bios.c
> --- arch/amd64/amd64/bios.c 15 Jul 2019 00:35:10 -0000 1.38
> +++ arch/amd64/amd64/bios.c 19 Jul 2019 11:45:11 -0000
> @@ -144,9 +144,11 @@ bios_attach(struct device *parent, struc
>      fixstring(scratch));
>   if ((smbios_get_string(&bios, sb->release,
>      scratch, sizeof(scratch))) != NULL) {
> - strlcpy(smbios_bios_date, fixstring(scratch),
> + char *s = fixstring(scratch) == NULL ? "" :
> +    fixstring(scratch);
> + strlcpy(smbios_bios_date, s,
>      sizeof(smbios_bios_date));
> - printf(" date %s", fixstring(scratch));
> + printf(" date %s", s);
>   }
>   }
>  
> Index: arch/i386/i386/bios.c
> ===================================================================
> RCS file: /cvs/src/sys/arch/i386/i386/bios.c,v
> retrieving revision 1.121
> diff -u -p -r1.121 bios.c
> --- arch/i386/i386/bios.c 15 Jul 2019 00:35:10 -0000 1.121
> +++ arch/i386/i386/bios.c 19 Jul 2019 11:58:57 -0000
> @@ -308,10 +308,11 @@ biosattach(struct device *parent, struct
>      fixstring(scratch));
>   if ((smbios_get_string(&bios, sb->release,
>      scratch, sizeof(scratch))) != NULL) {
> - strlcpy(smbios_bios_date,
> -    fixstring(scratch),
> + char *s = fixstring(scratch) == NULL ?
> +    "" : fixstring(scratch);
> + strlcpy(smbios_bios_date, s,
>      sizeof(smbios_bios_date));
> - printf(" date %s", fixstring(scratch));
> + printf(" date %s", s);
>   }
>   }
>   smbios_info(sc->sc_dev.dv_xname);
>

Reply | Threaded
Open this post in threaded view
|

Re: fix: NULL dereference in bios(4)

Jan Klemkow
On Sat, Jul 20, 2019 at 07:16:05PM +1000, Jonathan Gray wrote:

> On Fri, Jul 19, 2019 at 02:15:03PM +0200, Jan Klemkow wrote:
> > On Fri, Jul 19, 2019 at 09:13:38PM +1000, Jonathan Gray wrote:
> > > On Fri, Jul 19, 2019 at 01:07:34PM +0200, Jan Klemkow wrote:
> > > > fixstring() can return NULL and it does on one of my machines.
> > > > Here is s fix that checks for NULL and return an empty string.
> > > >
> > > > ok?
> > >
> > > If it returns NULL shouldn't the strlcpy and printf both be skipped?
> >
> > I was unsure about this.
> >
> > > You've also not included the i386 portion of this.
> >
> > I missed that.
> >
> > The diff below addresses all issues.
> >
> > ok?
>
> What argument does fixstring have in this case?

Ten spaces (10x 0x20).

> Does you dmesg not include a date in the 'bios0' line?

no.

> I was considering making this a pointer/dynamically allocated so it can
> be NULL.  If there is no valid date it should probably be NULL.
>
> >
> > Thanks,
> > Jan
> >
> > Index: arch/amd64/amd64/bios.c
> > ===================================================================
> > RCS file: /cvs/src/sys/arch/amd64/amd64/bios.c,v
> > retrieving revision 1.38
> > diff -u -p -r1.38 bios.c
> > --- arch/amd64/amd64/bios.c 15 Jul 2019 00:35:10 -0000 1.38
> > +++ arch/amd64/amd64/bios.c 19 Jul 2019 11:45:11 -0000
> > @@ -144,9 +144,11 @@ bios_attach(struct device *parent, struc
> >      fixstring(scratch));
> >   if ((smbios_get_string(&bios, sb->release,
> >      scratch, sizeof(scratch))) != NULL) {
> > - strlcpy(smbios_bios_date, fixstring(scratch),
> > + char *s = fixstring(scratch) == NULL ? "" :
> > +    fixstring(scratch);
> > + strlcpy(smbios_bios_date, s,
> >      sizeof(smbios_bios_date));
> > - printf(" date %s", fixstring(scratch));
> > + printf(" date %s", s);
> >   }
> >   }
> >  
> > Index: arch/i386/i386/bios.c
> > ===================================================================
> > RCS file: /cvs/src/sys/arch/i386/i386/bios.c,v
> > retrieving revision 1.121
> > diff -u -p -r1.121 bios.c
> > --- arch/i386/i386/bios.c 15 Jul 2019 00:35:10 -0000 1.121
> > +++ arch/i386/i386/bios.c 19 Jul 2019 11:58:57 -0000
> > @@ -308,10 +308,11 @@ biosattach(struct device *parent, struct
> >      fixstring(scratch));
> >   if ((smbios_get_string(&bios, sb->release,
> >      scratch, sizeof(scratch))) != NULL) {
> > - strlcpy(smbios_bios_date,
> > -    fixstring(scratch),
> > + char *s = fixstring(scratch) == NULL ?
> > +    "" : fixstring(scratch);
> > + strlcpy(smbios_bios_date, s,
> >      sizeof(smbios_bios_date));
> > - printf(" date %s", fixstring(scratch));
> > + printf(" date %s", s);
> >   }
> >   }
> >   smbios_info(sc->sc_dev.dv_xname);
> >

Reply | Threaded
Open this post in threaded view
|

Re: fix: NULL dereference in bios(4)

Jonathan Gray-11
On Mon, Jul 22, 2019 at 10:03:38AM +0200, Jan Klemkow wrote:

> On Sat, Jul 20, 2019 at 07:16:05PM +1000, Jonathan Gray wrote:
> > On Fri, Jul 19, 2019 at 02:15:03PM +0200, Jan Klemkow wrote:
> > > On Fri, Jul 19, 2019 at 09:13:38PM +1000, Jonathan Gray wrote:
> > > > On Fri, Jul 19, 2019 at 01:07:34PM +0200, Jan Klemkow wrote:
> > > > > fixstring() can return NULL and it does on one of my machines.
> > > > > Here is s fix that checks for NULL and return an empty string.
> > > > >
> > > > > ok?
> > > >
> > > > If it returns NULL shouldn't the strlcpy and printf both be skipped?
> > >
> > > I was unsure about this.
> > >
> > > > You've also not included the i386 portion of this.
> > >
> > > I missed that.
> > >
> > > The diff below addresses all issues.
> > >
> > > ok?
> >
> > What argument does fixstring have in this case?
>
> Ten spaces (10x 0x20).
>
> > Does you dmesg not include a date in the 'bios0' line?
>
> no.
>
> > I was considering making this a pointer/dynamically allocated so it can
> > be NULL.  If there is no valid date it should probably be NULL.

how about this?

Index: amd64/amd64/bios.c
===================================================================
RCS file: /cvs/src/sys/arch/amd64/amd64/bios.c,v
retrieving revision 1.38
diff -u -p -r1.38 bios.c
--- amd64/amd64/bios.c 15 Jul 2019 00:35:10 -0000 1.38
+++ amd64/amd64/bios.c 23 Jul 2019 12:00:27 -0000
@@ -92,6 +92,7 @@ bios_attach(struct device *parent, struc
  u_int8_t *p;
  int smbiosrev = 0;
  struct smbhdr *hdr = NULL;
+ char *sminfop;
 
  if (bios_efiinfo != NULL && bios_efiinfo->config_smbios != 0)
  hdr = smbios_find(PMAP_DIRECT_MAP(
@@ -144,9 +145,13 @@ bios_attach(struct device *parent, struc
     fixstring(scratch));
  if ((smbios_get_string(&bios, sb->release,
     scratch, sizeof(scratch))) != NULL) {
- strlcpy(smbios_bios_date, fixstring(scratch),
-    sizeof(smbios_bios_date));
- printf(" date %s", fixstring(scratch));
+ sminfop = fixstring(scratch);
+ if (sminfop != NULL) {
+ strlcpy(smbios_bios_date,
+    sminfop,
+    sizeof(smbios_bios_date));
+ printf(" date %s", sminfop);
+ }
  }
  }
 
Index: i386/i386/bios.c
===================================================================
RCS file: /cvs/src/sys/arch/i386/i386/bios.c,v
retrieving revision 1.121
diff -u -p -r1.121 bios.c
--- i386/i386/bios.c 15 Jul 2019 00:35:10 -0000 1.121
+++ i386/i386/bios.c 23 Jul 2019 12:00:51 -0000
@@ -245,6 +245,7 @@ biosattach(struct device *parent, struct
  for (va = ISA_HOLE_VADDR(SMBIOS_START);
     va < (u_int8_t *)ISA_HOLE_VADDR(SMBIOS_END); va+= 16) {
  struct smbhdr *sh = (struct smbhdr *)va;
+ char *sminfop;
  u_int8_t chksum;
  vaddr_t eva;
  paddr_t pa, end;
@@ -308,10 +309,13 @@ biosattach(struct device *parent, struct
     fixstring(scratch));
  if ((smbios_get_string(&bios, sb->release,
     scratch, sizeof(scratch))) != NULL) {
- strlcpy(smbios_bios_date,
-    fixstring(scratch),
-    sizeof(smbios_bios_date));
- printf(" date %s", fixstring(scratch));
+ sminfop = fixstring(scratch);
+ if (sminfop != NULL) {
+ strlcpy(smbios_bios_date,
+    sminfop,
+    sizeof(smbios_bios_date));
+ printf(" date %s", sminfop);
+ }
  }
  }
  smbios_info(sc->sc_dev.dv_xname);

Reply | Threaded
Open this post in threaded view
|

Re: fix: NULL dereference in bios(4)

Jan Klemkow
On Tue, Jul 23, 2019 at 10:05:58PM +1000, Jonathan Gray wrote:

> On Mon, Jul 22, 2019 at 10:03:38AM +0200, Jan Klemkow wrote:
> > On Sat, Jul 20, 2019 at 07:16:05PM +1000, Jonathan Gray wrote:
> > > On Fri, Jul 19, 2019 at 02:15:03PM +0200, Jan Klemkow wrote:
> > > > On Fri, Jul 19, 2019 at 09:13:38PM +1000, Jonathan Gray wrote:
> > > > > On Fri, Jul 19, 2019 at 01:07:34PM +0200, Jan Klemkow wrote:
> > > > > > fixstring() can return NULL and it does on one of my machines.
> > > > > > Here is s fix that checks for NULL and return an empty string.
> > > > > >
> > > > > > ok?
> > > > >
> > > > > If it returns NULL shouldn't the strlcpy and printf both be skipped?
> > > >
> > > > I was unsure about this.
> > > >
> > > > > You've also not included the i386 portion of this.
> > > >
> > > > I missed that.
> > > >
> > > > The diff below addresses all issues.
> > > >
> > > > ok?
> > >
> > > What argument does fixstring have in this case?
> >
> > Ten spaces (10x 0x20).
> >
> > > Does you dmesg not include a date in the 'bios0' line?
> >
> > no.
> >
> > > I was considering making this a pointer/dynamically allocated so it can
> > > be NULL.  If there is no valid date it should probably be NULL.
 
Ahh, sorry.  I don't get your suggestion on the first read.

> how about this?

Yes, this solution is fine for me and works on my hardware.

Thanks and OK,
Jan
 

> Index: amd64/amd64/bios.c
> ===================================================================
> RCS file: /cvs/src/sys/arch/amd64/amd64/bios.c,v
> retrieving revision 1.38
> diff -u -p -r1.38 bios.c
> --- amd64/amd64/bios.c 15 Jul 2019 00:35:10 -0000 1.38
> +++ amd64/amd64/bios.c 23 Jul 2019 12:00:27 -0000
> @@ -92,6 +92,7 @@ bios_attach(struct device *parent, struc
>   u_int8_t *p;
>   int smbiosrev = 0;
>   struct smbhdr *hdr = NULL;
> + char *sminfop;
>  
>   if (bios_efiinfo != NULL && bios_efiinfo->config_smbios != 0)
>   hdr = smbios_find(PMAP_DIRECT_MAP(
> @@ -144,9 +145,13 @@ bios_attach(struct device *parent, struc
>      fixstring(scratch));
>   if ((smbios_get_string(&bios, sb->release,
>      scratch, sizeof(scratch))) != NULL) {
> - strlcpy(smbios_bios_date, fixstring(scratch),
> -    sizeof(smbios_bios_date));
> - printf(" date %s", fixstring(scratch));
> + sminfop = fixstring(scratch);
> + if (sminfop != NULL) {
> + strlcpy(smbios_bios_date,
> +    sminfop,
> +    sizeof(smbios_bios_date));
> + printf(" date %s", sminfop);
> + }
>   }
>   }
>  
> Index: i386/i386/bios.c
> ===================================================================
> RCS file: /cvs/src/sys/arch/i386/i386/bios.c,v
> retrieving revision 1.121
> diff -u -p -r1.121 bios.c
> --- i386/i386/bios.c 15 Jul 2019 00:35:10 -0000 1.121
> +++ i386/i386/bios.c 23 Jul 2019 12:00:51 -0000
> @@ -245,6 +245,7 @@ biosattach(struct device *parent, struct
>   for (va = ISA_HOLE_VADDR(SMBIOS_START);
>      va < (u_int8_t *)ISA_HOLE_VADDR(SMBIOS_END); va+= 16) {
>   struct smbhdr *sh = (struct smbhdr *)va;
> + char *sminfop;
>   u_int8_t chksum;
>   vaddr_t eva;
>   paddr_t pa, end;
> @@ -308,10 +309,13 @@ biosattach(struct device *parent, struct
>      fixstring(scratch));
>   if ((smbios_get_string(&bios, sb->release,
>      scratch, sizeof(scratch))) != NULL) {
> - strlcpy(smbios_bios_date,
> -    fixstring(scratch),
> -    sizeof(smbios_bios_date));
> - printf(" date %s", fixstring(scratch));
> + sminfop = fixstring(scratch);
> + if (sminfop != NULL) {
> + strlcpy(smbios_bios_date,
> +    sminfop,
> +    sizeof(smbios_bios_date));
> + printf(" date %s", sminfop);
> + }
>   }
>   }
>   smbios_info(sc->sc_dev.dv_xname);