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.
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.
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:
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:
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:
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:
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.