Daniel Hansel wrote:
> Hi,
>
> here are fixes for several security issues intruduced in the new
files for JIT support.
>
> Please review and give comments on that.
> Thanks in advance.
Hi, Daniel,
It looks like you've addressed all of the concerns our security person
identified. I have just a couple comments (see below).
Thanks.
-Maynard
>
> Kind regards,
> Daniel
>
>
>
[snip]
> diff -prauN -X ../workspaces/eclipse/excludelist.txt
../workspaces/eclipse/oprofile/opjitconv/debug_line.c
oprofile_work_20080318/opjitconv/debug_line.c
> --- ../workspaces/eclipse/oprofile/opjitconv/debug_line.c
2008-01-22 23:48:50.000000000 +0100
> +++ oprofile_work_20080318/opjitconv/debug_line.c 2008-03-18
14:10:07.000000000 +0100
> @@ -279,7 +279,7 @@ static ubyte get_special_opcode(struct d
> unsigned int temp;
> unsigned long delta_addr;
>
> - /* See TIS DWARF Debugging Information Format version 2.0 §
6.2.5.1 */
> + /* See TIS DWARF Debugging Information Format version 2.0 §
6.2.5.1 */
>
> temp = (line->lineno - last_lineno) -
> default_debug_line_header.line_base;
> @@ -454,8 +454,8 @@ int init_debug_line_info(bfd * abfd)
> size_t i;
> void const * data = rec + 1;
> struct debug_line_info * dbg_line =
> - xmalloc(rec->nr_entry *
> - sizeof(struct debug_line_info));
> + xmalloc((u32)rec->nr_entry *
> + (u32)sizeof(struct debug_line_info));
I don't see the need for this change since rec->nr_entry and the return
value of sizeof are both size_t (unsigned integers). Am I missing
something?
> for (i = 0; i < rec->nr_entry; ++i) {
> dbg_line[i].vma = *(unsigned long *)data;
> data += sizeof(unsigned long);
> diff -prauN -X ../workspaces/eclipse/excludelist.txt
../workspaces/eclipse/oprofile/opjitconv/jitsymbol.c
oprofile_work_20080318/opjitconv/jitsymbol.c
> --- ../workspaces/eclipse/oprofile/opjitconv/jitsymbol.c
2008-01-09 10:54:42.000000000 +0100
> +++ oprofile_work_20080318/opjitconv/jitsymbol.c 2008-03-18
14:10:46.000000000 +0100
> @@ -8,6 +8,7 @@
> * @author Jens Wilke
> * @Modifications Maynard Johnson
> * @Modifications Philippe Elie
> + * @Modifications Daniel Hansel
> *
[snip]
> static void split_entry(struct jitentry * split, struct jitentry
const * keep)
> {
> @@ -243,9 +251,20 @@ static void split_entry(struct jitentry
> if (end_addr_split > end_addr_keep) {
> struct jitentry * new_entry =
> xcalloc(1, sizeof(struct jitentry));
> - char * s = xmalloc(strlen(split->symbol_name) + 3);
> - strcpy(s, split->symbol_name);
> - strcat(s, "#1");
> + char * s = NULL;
> + + /* Check for max. length to avoid possible integer
overflow. */
> + if (strlen(split->symbol_name) > SIZE_MAX - 3) {
WOW! I don't think we should be trying to allocate that much memory.
If we find a symbol name that's this long, something bad is happening.
I would spit out an error and quit.
> + s = xmalloc(SIZE_MAX);
> + strncpy(s, split->symbol_name, SIZE_MAX - 3);
> + s[SIZE_MAX - 3] = '\0';
> + strncat(s, "#1", 2);
> + } else {
> + s = xmalloc(strlen(split->symbol_name) + 3);
> + strcpy(s, split->symbol_name);
> + strcat(s, "#1");
> + }
> + new_entry->vma = end_addr_keep;
> new_entry->code_size = end_addr_split - end_addr_keep;
> new_entry->symbol_name = s;
> @@ -263,9 +282,20 @@ static void split_entry(struct jitentry
> }
> // do we need a left part?
> if (start_addr_split < start_addr_keep) {
> - char * s = xmalloc(strlen(split->symbol_name) + 3);
> - strcpy(s, split->symbol_name);
> - strcat(s, "#0");
> + char * s = NULL;
> + + /* Check for max. length to avoid possible integer
overflow. */
> + if (strlen(split->symbol_name) > SIZE_MAX - 3) {
Same comment as above.
> + s = xmalloc(SIZE_MAX);
> + strncpy(s, split->symbol_name, SIZE_MAX - 3);
> + s[SIZE_MAX - 3] = '\0';
> + strncat(s, "#0", 2);
> + } else {
> + s = xmalloc(strlen(split->symbol_name) + 3);
> + strcpy(s, split->symbol_name);
> + strcat(s, "#0");
> + }
> + split->code_size = start_addr_keep - start_addr_split;
[snip]
|