What’s Hiding Inside the GNU Boot Loader? Searching for Bugs in Grub

PVS-Studio analyzer continues to explore and adapt to the Linux platform. Today we will take a look at the bugs that the tool managed to find in the Grub boot loader.

image1

Introduction

In this article, we will talk about the results of analysis of the boot loader for Unix-like operating systems, known as Grub. This program was developed by Erich Boleyn and comes as part of the GNU Project. GRUB is a reference boot loader implementation compliant with the Multiboot specification and is able to boot any compliant operating system.

The Grub project is written in C and has been already checked by other analyzers, including Coverity, so you wouldn’t expect to find any unchecked code fragments in a project like that. PVS-Studio analyzer, however, did manage to catch a few interesting bugs.

Analysis results

Typos are one of the most common errors in programs. Even skillful developers make them every now and then. So, it’s just right to start with typos.

Constant name mistyped

typedef enum
{
  GRUB_PARSER_STATE_TEXT = 1,
  GRUB_PARSER_STATE_ESC,
  GRUB_PARSER_STATE_QUOTE,
  GRUB_PARSER_STATE_DQUOTE,
  ....
} grub_parser_state_t;

char * grub_normal_do_completion (....)
{
  ....
  if (*escstr == ' ' 
      && cmdline_state != GRUB_PARSER_STATE_QUOTE
      && cmdline_state != GRUB_PARSER_STATE_QUOTE)  // <=
        *(newstr++) = '\\';
  ....
}

PVS-Studio diagnostic message: V501 There are identical sub-expressions ‘cmdline_state != GRUB_PARSER_STATE_QUOTE’ to the left and to the right of the ‘&&’ operator. completion.c 502

Typos in similarly looking names of constants are quite a common issue. In the example above, the programmer must have intended to compare the value of cmdline_state with the GRUB_PARSER_STATE_DQUOTE constant instead of comparing it with GRUB_PARSER_STATE_QUOTE one more time.

Register name mistyped

struct grub_bios_int_registers
{
  grub_uint32_t eax;
  grub_uint16_t es;
  grub_uint16_t ds;
  grub_uint16_t flags;
  grub_uint16_t dummy;
  grub_uint32_t ebx;
  grub_uint32_t ecx;
  grub_uint32_t edi;
  grub_uint32_t esi;
  grub_uint32_t edx;
};

grub_vbe_status_t 
grub_vbe_bios_getset_dac_palette_width (....)
{
  struct grub_bios_int_registers regs;

  regs.eax = 0x4f08;
  regs.ebx = (*dac_mask_size & 0xff) >> 8;
  regs.ebx = set ? 1 : 0;                 // <=
  ....
}

PVS-Studio diagnostic message: V519 The ‘regs.ebx’ variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 312, 313. vbe.c 313

The regs struct is a wrapper for handling registers dealing with memory. Given the registers’ similar names, it’s very easy to make a mistake. In the example above, some other register should be used instead of ebx in the second case. Without knowing the specifics of this code, I can’t say for sure how exactly it should be fixed. The analyzer’s main purpose is to point out a problem, while finding a solution is the developer’s job. This is the reason why static analysis is most needed while you are only going through the development process.

Meaningless assignment

static void free_subchunk (....)
{
  switch (subchu->type)
    {
    case CHUNK_TYPE_REGION_START:
      {
       grub_mm_region_t r1, r2, *rp;
       ....
       if (*rp)
       {
        ....
       }
       else
       {
         r1->pre_size = pre_size;
         r1->size = (r2 - r1) * sizeof (*r2);
         for (rp = &grub_mm_base; *rp; rp = &((*rp)->next))
           if ((*rp)->size > r1->size)
             break;
         r1->next = *rp;               // <=
         *rp = r1->next;               // <=
         h = (grub_mm_header_t) (r1 + 1);
         r1->first = h;
         h->next = h;
         h->magic = GRUB_MM_FREE_MAGIC;
         h->size = (r2 - r1 - 1);
       }
       ....
       if (r2)
       {
         ....
         r2->size += r1->size;
         ....
         hl2->next = r2->first;
         r2->first = r1->first;
         hl->next = r2->first;
         *rp = (*rp)->next;
         ....
       } 
       ....
      }
     ....
    }
  ....
}

PVS-Studio diagnostic message: V587 An odd sequence of assignments of this kind: A = B; B = A;. Check lines: 338, 339. relocator.c 339

Errors of this type are not that common. I’m not sure what exactly the programmer intended this code to look like. A field is assigned a pointer stored in the *rp variable, while the next line contains a reverse operation: the r1->next pointer is assigned to the *rp variable. Code like that doesn’t make sense as the *rp variable is already storing that value. Just looking at the code, you can’t figure out if this is an error or just a superfluous operation. I believe it’s an error.

Incorrect argument for a memset

static void setup (....)
{
  ....
  struct grub_boot_blocklist *first_block, *block;
  ....
  /* Clean out the blocklists.  */
  block = first_block;
  while (block->len)
    {
     grub_memset (block, 0, sizeof (block)); // <=
     block--;
     ....
    }
  ....
}

PVS-Studio diagnostic message: V579 The grub_memset function receives the pointer and its size as arguments. It is possibly a mistake. Inspect the third argument. grub-setup.c 500

Functions for low-level memory management are a hotbed of typos. When using them, programmers often make mistakes when computing the buffer size. In this example, too, the grub_memset function receives the pointer size instead of the block buffer’s size as the third argument, which results in incomplete clearing of block. This is what the fixed code should look like:

grub_memset (block, 0, sizeof (*block));

A few more similar issues:

  • V579 The grub_memset function receives the pointer and its size as arguments. It is possibly a mistake. Inspect the third argument. mmap.c 148
  • V579 The grub_memset function receives the pointer and its size as arguments. It is possibly a mistake. Inspect the third argument. mmap.c 165

Incorrect memory cleanup

static gcry_err_code_t do_arcfour_setkey (....)
{
  byte karr[256];
  ....
  for (i=0; i < 256; i++ )
    karr[i] = key[i%keylen];
  ....
  memset( karr, 0, 256 );   // <=

  return GPG_ERR_NO_ERROR;
}

PVS-Studio diagnostic message: V597 The compiler could delete the ‘memset’ function call, which is used to flush ‘karr’ buffer. The RtlSecureZeroMemory() function should be used to erase the private data. arcfour.c 108

It’s a bad idea to use the memset function to free memory in this example. Execution leaves the function immediately after the call to memset, and if the buffer is not used anymore, the compiler may remove the call to memset when building the program. To avoid it, use the memset_s function instead.

The analyzer issued a few more warnings related to memory cleanup:

  • V597 The compiler could delete the ‘memset’ function call, which is used to flush ‘buf’ object. The RtlSecureZeroMemory() function should be used to erase the private data. grub-mkpasswd-pbkdf2.c 209
  • V597 The compiler could delete the ‘memset’ function call, which is used to flush ‘bufhex’ object. The RtlSecureZeroMemory() function should be used to erase the private data. grub-mkpasswd-pbkdf2.c 210
  • V597 The compiler could delete the ‘memset’ function call, which is used to flush ‘salt’ object. The RtlSecureZeroMemory() function should be used to erase the private data. grub-mkpasswd-pbkdf2.c 213
  • V597 The compiler could delete the ‘memset’ function call, which is used to flush ‘salthex’ object. The RtlSecureZeroMemory() function should be used to erase the private data. grub-mkpasswd-pbkdf2.c 214
  • V597 The compiler could delete the ‘memset’ function call, which is used to flush ‘buf’ object. The RtlSecureZeroMemory() function should be used to erase the private data. grub-mkpasswd-pbkdf2.c 231
  • V597 The compiler could delete the ‘memset’ function call, which is used to flush ‘bufhex’ object. The RtlSecureZeroMemory() function should be used to erase the private data. grub-mkpasswd-pbkdf2.c 232
  • V597 The compiler could delete the ‘memset’ function call, which is used to flush ‘salt’ object. The RtlSecureZeroMemory() function should be used to erase the private data. grub-mkpasswd-pbkdf2.c 235
  • V597 The compiler could delete the ‘memset’ function call, which is used to flush ‘salthex’ object. The RtlSecureZeroMemory() function should be used to erase the private data. grub-mkpasswd-pbkdf2.c 236
  • V597 The compiler could delete the ‘memset’ function call, which is used to flush ‘pass2’ buffer. The RtlSecureZeroMemory() function should be used to erase the private data. grub-mkpasswd-pbkdf2.c 166
  • V597 The compiler could delete the ‘memset’ function call, which is used to flush ‘pass1’ buffer. The RtlSecureZeroMemory() function should be used to erase the private data. grub-mkpasswd-pbkdf2.c 205

Superfluous operation

Int main (int argc, char *argv[])
{
  ....
  {
    FILE *f;
    size_t rd;
    f = fopen ("/dev/urandom", "rb");
    if (!f)
    {
      memset (pass1, 0, sizeof (pass1));
      free (buf);
      free (bufhex);
      free (salthex);
      free (salt);
      fclose (f);                     // <=
      ....
    }
    ....
    fclose (f);
  }
  ....
}

PVS-Studio diagnostic message: V575 The null pointer is passed into ‘fclose’ function. Inspect the first argument. grub-mkpasswd-pbkdf2.c 184

If the file fails to open, the temporary variables are freed. For some reason, the programmer also added a call to the fclose function, which is used for the file closing, to the conditional block. The conditional expression, however, checks that the file hasn’t opened, so there’s no need to close it, while passing NULL to a function leads to invoking a handler for invalid parameters, as specified by the documentation. The program’s further behavior depends on the handler’s settings. The code above is incorrect anyway and has to be fixed by removing the call to the fclose function in the conditional statement.

One more suspicious fragment found by diagnostic V575:

  • V575 The null pointer is passed into ‘free’ function. Inspect the first argument. grub-setup.c 1187

Unused value

static grub_err_t grub_video_cirrus_setup (....)
{
  ....
  if (CIRRUS_APERTURE_SIZE >= 2 * framebuffer.page_size)
    err = grub_video_fb_setup (mode_type, mode_mask,
                   &framebuffer.mode_info,
                   framebuffer.ptr,
                   doublebuf_pageflipping_set_page,
                   framebuffer.ptr + framebuffer.page_size);
  else
    err = grub_video_fb_setup (mode_type, mode_mask,
                   &framebuffer.mode_info,
                   framebuffer.ptr, 0, 0);

  err = grub_video_cirrus_set_palette (0, 
                       GRUB_VIDEO_FBSTD_NUMCOLORS,
                       grub_video_fbstd_colors);
  return err;
}

PVS-Studio diagnostic message: V519 The ‘err’ variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 448, 460. cirrus.c 460

There is actually nothing critical about this fragment. The programmer seems to assume that the grub_video_fb_setup function can’t return an error. If it really can’t, then why do they save its return value in a variable when that value is immediately overwritten anyway? Perhaps the variable is simply used to monitor the value during debugging, but it may also be a sign of some important check missing here. In any case, this code needs to be checked and rewritten.

Another suspicious fragment:

  • V519 The ‘err’ variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 368, 380. bochs.c 380

Conclusion

Even well-tested projects have bugs in them. Static analysis brings benefits for the software at every stage of development. While we are approaching the release date of PVS-Studio for Linux, take a look at the analysis results for other projects.

By Alexander Chibisov

Leave a Reply

Fill in your details below or click an icon to log in:

WordPress.com Logo

You are commenting using your WordPress.com account. Log Out / Change )

Twitter picture

You are commenting using your Twitter account. Log Out / Change )

Facebook photo

You are commenting using your Facebook account. Log Out / Change )

Google+ photo

You are commenting using your Google+ account. Log Out / Change )

Connecting to %s