[PATCH 0/7] Fix coverity uninitialized scalar variable bugs in grub-core

[PATCH 0/7] Fix coverity uninitialized scalar variable bugs in grub-core

[Alec Brown]

Coverity identified multiple uninitialized scalar variable bugs in multiple
components of the grub-core. These patches address these issues.

The Coverity bugs being addressed are:
CID 375026
CID 375028
CID 375030
CID 375031
CID 375033
CID 375035
CID 375036

Alec Brown (7):
      grub-core/loader/i386/bsd.c: Fix uninitialized scalar variable
      grub-core/loader/i386/pc/linux.c: Fix uninitialized scalar variable
      grub-core/net/arp.c: Fix uninitialized scalar variable
      grub-core/loader/i386/xnu.c: Fix uninitialized scalar variable
      grub-core/net/net.c: Fix uninitialized scalar variable
      grub-core/loader/i386/xnu.c: Fix uninitialized scalar variable
      grub-core/net/bootp.c: Fix uninitialized scalar variable

 grub-core/loader/i386/bsd.c      | 2 +-
 grub-core/loader/i386/pc/linux.c | 2 +-
 grub-core/loader/i386/xnu.c      | 4 ++--
 grub-core/net/arp.c              | 3 ++-
 grub-core/net/bootp.c            | 2 +-
 grub-core/net/net.c              | 2 +-
 6 files changed, 8 insertions(+), 7 deletions(-)

[PATCH 1/7] grub-core/loader/i386/bsd.c: Fix uninitialized scalar variable

[Alec Brown]

In the function grub_netbsd_setup_video(), struct grub_netbsd_btinfo_framebuf
params is called but isn't being initialized. To prevent contents of this
structure from being filled with junk data from the stack, we can initialize it to 0
by setting params to {}.

Fixes: CID 375026

Signed-off-by: Alec Brown <alec.r.brown@oracle.com>
---
 grub-core/loader/i386/bsd.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/grub-core/loader/i386/bsd.c b/grub-core/loader/i386/bsd.c
index 5f3290ce1..b99344556 100644
--- a/grub-core/loader/i386/bsd.c
+++ b/grub-core/loader/i386/bsd.c
@@ -929,7 +929,7 @@ grub_netbsd_setup_video (void)
   struct grub_video_mode_info mode_info;
   void *framebuffer;
   const char *modevar;
-  struct grub_netbsd_btinfo_framebuf params;
+  struct grub_netbsd_btinfo_framebuf params = {};
   grub_err_t err;
   grub_video_driver_id_t driv_id;
 
-- 
2.27.0

[PATCH 2/7] grub-core/loader/i386/pc/linux.c: Fix uninitialized scalar variable

[Alec Brown]

In the function grub_linux16_boot(), struct grub_relocator16_state state is
called but isn't being initialized. To prevent contents of this structure from
being filled with junk data from the stack, we can initialize it to 0 by setting
state to {};

Fixes: CID 375028

Signed-off-by: Alec Brown <alec.r.brown@oracle.com>
---
 grub-core/loader/i386/pc/linux.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/grub-core/loader/i386/pc/linux.c b/grub-core/loader/i386/pc/linux.c
index 2a2995201..808818d5f 100644
--- a/grub-core/loader/i386/pc/linux.c
+++ b/grub-core/loader/i386/pc/linux.c
@@ -55,7 +55,7 @@ static grub_err_t
 grub_linux16_boot (void)
 {
   grub_uint16_t segment;
-  struct grub_relocator16_state state;
+  struct grub_relocator16_state state = {};
 
   segment = grub_linux_real_target >> 4;
   state.gs = state.fs = state.es = state.ds = state.ss = segment;
-- 
2.27.0

[PATCH 3/7] grub-core/net/arp.c: Fix uninitialized scalar variable

[Alec Brown]

In the function grub_net_arp_receive(), grub_net_network_level_address_t
sender_addr and target_addr are being called but aren't being initialized. To
prevent contents of these structures from being filled with junk data from the
stack, we can initialize them to 0 by setting sender_addr and target_addr to {}.

Fixes: CID 375030

Signed-off-by: Alec Brown <alec.r.brown@oracle.com>
---
 grub-core/net/arp.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/grub-core/net/arp.c b/grub-core/net/arp.c
index 54306e3b1..2b3765932 100644
--- a/grub-core/net/arp.c
+++ b/grub-core/net/arp.c
@@ -115,7 +115,8 @@ grub_net_arp_receive (struct grub_net_buff *nb, struct grub_net_card *card,
                       grub_uint16_t *vlantag)
 {
   struct arppkt *arp_packet = (struct arppkt *) nb->data;
-  grub_net_network_level_address_t sender_addr, target_addr;
+  grub_net_network_level_address_t sender_addr = {};
+  grub_net_network_level_address_t target_addr = {};
   grub_net_link_level_address_t sender_mac_addr;
   struct grub_net_network_level_interface *inf;
 
-- 
2.27.0

[PATCH 4/7] grub-core/loader/i386/xnu.c: Fix uninitialized scalar variable

[Alec Brown]

In the function grub_xnu_boot_resume(), struct grub_relocator32_state state is
called but isn't being initialized. To prevent contents of this structure from
being filled with junk data from the stack, we can initialize it to 0 by setting
state to {}.

Fixes: CID 375031

Signed-off-by: Alec Brown <alec.r.brown@oracle.com>
---
 grub-core/loader/i386/xnu.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/grub-core/loader/i386/xnu.c b/grub-core/loader/i386/xnu.c
index a70093607..caab5cfa6 100644
--- a/grub-core/loader/i386/xnu.c
+++ b/grub-core/loader/i386/xnu.c
@@ -805,7 +805,7 @@ grub_cpu_xnu_fill_devicetree (grub_uint64_t *fsbfreq_out)
 grub_err_t
 grub_xnu_boot_resume (void)
 {
-  struct grub_relocator32_state state;
+  struct grub_relocator32_state state = {};
 
   state.esp = grub_xnu_stack;
   state.ebp = grub_xnu_stack;
-- 
2.27.0

[PATCH 5/7] grub-core/net/net.c: Fix uninitialized scalar variable

[Alec Brown]

In the function grub_net_ipv6_get_link_local(), grub_net_network_level_address_t
addr is called but isn't being initialized. To prevent contents of this
structure from being filled with junk data from the stack, we can initialize it
to 0 by setting addr to {};

Fixes: CID 375033

Signed-off-by: Alec Brown <alec.r.brown@oracle.com>
---
 grub-core/net/net.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/grub-core/net/net.c b/grub-core/net/net.c
index 4d3eb5c1a..4e93365a7 100644
--- a/grub-core/net/net.c
+++ b/grub-core/net/net.c
@@ -287,7 +287,7 @@ grub_net_ipv6_get_link_local (struct grub_net_card *card,
   struct grub_net_network_level_interface *inf;
   char *name;
   char *ptr;
-  grub_net_network_level_address_t addr;
+  grub_net_network_level_address_t addr = {};
 
   addr.type = GRUB_NET_NETWORK_LEVEL_PROTOCOL_IPV6;
   addr.ipv6[0] = grub_cpu_to_be64_compile_time (0xfe80ULL << 48);
-- 
2.27.0

[PATCH 6/7] grub-core/loader/i386/xnu.c: Fix uninitialized scalar variable

[Alec Brown]

In the function grub_xnu_boot(), struct grub_relocator32_state state is called
but isn't being initialized. To prevent contents of this structure from being
filled with junk data from the stack, we can initialize it to 0 by setting state
to {}.

Fixes: CID 375035

Signed-off-by: Alec Brown <alec.r.brown@oracle.com>
---
 grub-core/loader/i386/xnu.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/grub-core/loader/i386/xnu.c b/grub-core/loader/i386/xnu.c
index caab5cfa6..c956942a2 100644
--- a/grub-core/loader/i386/xnu.c
+++ b/grub-core/loader/i386/xnu.c
@@ -960,7 +960,7 @@ grub_xnu_boot (void)
   grub_addr_t devtree_target;
   grub_size_t devtreelen;
   int i;
-  struct grub_relocator32_state state;
+  struct grub_relocator32_state state = {};
   grub_uint64_t fsbfreq = 100000000;
   int v2 = (grub_xnu_darwin_version >= 11);
   grub_uint32_t efi_system_table = 0;
-- 
2.27.0

[PATCH 7/7] grub-core/net/bootp.c: Fix uninitialized scalar variable

[Alec Brown]

In the function grub_net_configure_by_dhcp_ack(),
grub_net_network_level_address_t addr is called but isn't being initialized. To
prevent contents of this structure from being filled with junk data from the
stack, we can initialize it to 0 by setting addr to {}.

Fixes: CID 375036

Signed-off-by: Alec Brown <alec.r.brown@oracle.com>
---
 grub-core/net/bootp.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/grub-core/net/bootp.c b/grub-core/net/bootp.c
index 6fb562702..708d601df 100644
--- a/grub-core/net/bootp.c
+++ b/grub-core/net/bootp.c
@@ -232,7 +232,7 @@ grub_net_configure_by_dhcp_ack (const char *name,
 				grub_size_t size,
 				int is_def, char **device, char **path)
 {
-  grub_net_network_level_address_t addr;
+  grub_net_network_level_address_t addr = {};
   grub_net_link_level_address_t hwaddr;
   struct grub_net_network_level_interface *inter;
   int mask = -1;
-- 
2.27.0

Re: [PATCH 0/7] Fix coverity uninitialized scalar variable bugs in grub-core

[Darren Kenny]

Hi Alec,

Thanks for doing these changes, it all looks good, so for the series:

Reviewed-by: Darren Kenny <darren.kenny@oracle.com>

Thanks,

Darren.

On Tuesday, 2022-03-15 at 16:24:02 -04, Alec Brown wrote:
> Coverity identified multiple uninitialized scalar variable bugs in multiple
> components of the grub-core. These patches address these issues.
>
> The Coverity bugs being addressed are:
> CID 375026
> CID 375028
> CID 375030
> CID 375031
> CID 375033
> CID 375035
> CID 375036
>
> Alec Brown (7):
>       grub-core/loader/i386/bsd.c: Fix uninitialized scalar variable
>       grub-core/loader/i386/pc/linux.c: Fix uninitialized scalar variable
>       grub-core/net/arp.c: Fix uninitialized scalar variable
>       grub-core/loader/i386/xnu.c: Fix uninitialized scalar variable
>       grub-core/net/net.c: Fix uninitialized scalar variable
>       grub-core/loader/i386/xnu.c: Fix uninitialized scalar variable
>       grub-core/net/bootp.c: Fix uninitialized scalar variable
>
>  grub-core/loader/i386/bsd.c      | 2 +-
>  grub-core/loader/i386/pc/linux.c | 2 +-
>  grub-core/loader/i386/xnu.c      | 4 ++--
>  grub-core/net/arp.c              | 3 ++-
>  grub-core/net/bootp.c            | 2 +-
>  grub-core/net/net.c              | 2 +-
>  6 files changed, 8 insertions(+), 7 deletions(-)

Re: [PATCH 5/7] grub-core/net/net.c: Fix uninitialized scalar variable

[Glenn Washburn]

On Tue, 15 Mar 2022 16:24:07 -0400
Alec Brown <alec.r.brown@oracle.com> wrote:

> In the function grub_net_ipv6_get_link_local(), grub_net_network_level_address_t
> addr is called but isn't being initialized. To prevent contents of this
> structure from being filled with junk data from the stack, we can initialize it
> to 0 by setting addr to {};
> 
> Fixes: CID 375033
> 
> Signed-off-by: Alec Brown <alec.r.brown@oracle.com>
> ---
>  grub-core/net/net.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/grub-core/net/net.c b/grub-core/net/net.c
> index 4d3eb5c1a..4e93365a7 100644
> --- a/grub-core/net/net.c
> +++ b/grub-core/net/net.c
> @@ -287,7 +287,7 @@ grub_net_ipv6_get_link_local (struct grub_net_card *card,
>    struct grub_net_network_level_interface *inf;
>    char *name;
>    char *ptr;
> -  grub_net_network_level_address_t addr;
> +  grub_net_network_level_address_t addr = {};
>  
>    addr.type = GRUB_NET_NETWORK_LEVEL_PROTOCOL_IPV6;
>    addr.ipv6[0] = grub_cpu_to_be64_compile_time (0xfe80ULL << 48);

This seems not quite necessary. The local "addr" is initialized just
below its initialization, so "junk" data doesn't matter. Only the
"option" member is not initialized, so we could just add another line to
initialize that.

The "{}" syntax seems to not be used much either, "{0}" being
preferred, but also not used much.

I think I remember Vladimir saying that GRUB doesn't use initializers,
but there are some in the code, so perhaps this isn't a thing
anymore.

Another option, which would be my preference, would be to move the 3
lines below the declaraction of "addr" into the initializer and use
C99's designated initializer, so something like:

  grub_net_network_level_address_t addr = {
    .type = GRUB_NET_NETWORK_LEVEL_PROTOCOL_IPV6,
    .ipv6 = {
      grub_cpu_to_be64_compile_time (0xfe80ULL << 48),
      grub_net_ipv6_get_id (hwaddr)
    }
  };

There is precedent for using function calls in initializers
(net/dns.c:453), but its also rare.

Daniel, do you have a preference here?

Glenn

Re: [PATCH 5/7] grub-core/net/net.c: Fix uninitialized scalar variable

[Daniel Kiper]

On Tue, Mar 15, 2022 at 08:16:01PM -0500, Glenn Washburn wrote:
> On Tue, 15 Mar 2022 16:24:07 -0400
> Alec Brown <alec.r.brown@oracle.com> wrote:
>
> > In the function grub_net_ipv6_get_link_local(), grub_net_network_level_address_t
> > addr is called but isn't being initialized. To prevent contents of this
> > structure from being filled with junk data from the stack, we can initialize it
> > to 0 by setting addr to {};
> >
> > Fixes: CID 375033
> >
> > Signed-off-by: Alec Brown <alec.r.brown@oracle.com>
> > ---
> >  grub-core/net/net.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/grub-core/net/net.c b/grub-core/net/net.c
> > index 4d3eb5c1a..4e93365a7 100644
> > --- a/grub-core/net/net.c
> > +++ b/grub-core/net/net.c
> > @@ -287,7 +287,7 @@ grub_net_ipv6_get_link_local (struct grub_net_card *card,
> >    struct grub_net_network_level_interface *inf;
> >    char *name;
> >    char *ptr;
> > -  grub_net_network_level_address_t addr;
> > +  grub_net_network_level_address_t addr = {};
> >
> >    addr.type = GRUB_NET_NETWORK_LEVEL_PROTOCOL_IPV6;
> >    addr.ipv6[0] = grub_cpu_to_be64_compile_time (0xfe80ULL << 48);
>
> This seems not quite necessary. The local "addr" is initialized just
> below its initialization, so "junk" data doesn't matter. Only the
> "option" member is not initialized, so we could just add another line to
> initialize that.

Yeah, I chatted about that with Alec and he will fix it.

> The "{}" syntax seems to not be used much either, "{0}" being
> preferred, but also not used much.

Good point! I asked Alec to fix it too.

> I think I remember Vladimir saying that GRUB doesn't use initializers,
> but there are some in the code, so perhaps this isn't a thing
> anymore.

If Coverity complains I think we should make it happy. I think it is
better to be on safe side than sorry.

> Another option, which would be my preference, would be to move the 3
> lines below the declaraction of "addr" into the initializer and use
> C99's designated initializer, so something like:
>
>   grub_net_network_level_address_t addr = {
>     .type = GRUB_NET_NETWORK_LEVEL_PROTOCOL_IPV6,
>     .ipv6 = {
>       grub_cpu_to_be64_compile_time (0xfe80ULL << 48),
>       grub_net_ipv6_get_id (hwaddr)
>     }
>   };

I think it is rather an overkill here.

Daniel