未加星标

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

字体大小 | |
[系统(linux) 所属分类 系统(linux) | 发布者 店小二04 | 时间 2016 | 作者 红领巾 ] 0人收藏点击收藏

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.


What’s Hiding Inside the GNU Boot Loader? Searching for Bugs in Grub
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

本文系统(linux)相关术语:linux系统 鸟哥的linux私房菜 linux命令大全 linux操作系统

主题: LinuxUB
分页:12
转载请注明
本文标题:What’s Hiding Inside the GNU Boot Loader? Searching for Bugs in Grub
本站链接:http://www.codesec.net/view/479874.html
分享请点击:


1.凡CodeSecTeam转载的文章,均出自其它媒体或其他官网介绍,目的在于传递更多的信息,并不代表本站赞同其观点和其真实性负责;
2.转载的文章仅代表原创作者观点,与本站无关。其原创性以及文中陈述文字和内容未经本站证实,本站对该文以及其中全部或者部分内容、文字的真实性、完整性、及时性,不作出任何保证或承若;
3.如本站转载稿涉及版权等问题,请作者及时联系本站,我们会及时处理。
登录后可拥有收藏文章、关注作者等权限...
技术大类 技术大类 | 系统(linux) | 评论(0) | 阅读(62)