ubsan for kpatch

[sumanthkorikkar]

Hi all,

This is regarding ubsan support for kpatch. ubuntu recently queried regarding this option on s390. I quickly tested it on x86 and it seems to fail on x86 in a similar way.

I did perform initial preliminary analysis. These are my current findings. Debug data: 2a. buggy program:

#include <stdio.h>
int main(int argc, char **argv) {
        int arr[100];
        arr[101] = 1;
        printf("arr[101] = %d", arr[101]);
        return 0;
}

2b. Disassembly and relocs:

  1a:   50 10 b0 ac             st      %r1,172(%r11)
        int arr[100];
        arr[101] = 1;
  1e:   a7 39 00 65             lghi    %r3,101
  22:   c0 20 00 00 00 00       larl    %r2,22 <main+0x22>
                        24: R_390_PC32DBL       .data..Lubsan_data1+0x2
  28:   c0 e5 00 00 00 00       brasl   %r14,28 <main+0x28>
                        2a: R_390_PLT32DBL      __ubsan_handle_out_of_bounds+0x2

2c. .data.*Lubsan represents struct out_of_bounds_data, which is declared in linux/lib/ubsan.h

struct out_of_bounds_data {
      struct source_location location;
      struct type_descriptor *array_type;
      struct type_descriptor *index_type;
};

 struct source_location {
      const char *file_name;
      union {
            unsigned long reported;
            struct {
                  u32 line;
                  u32 column;
            };
      };
};

2d. Disassembly of section .data..Lubsan_data1:

0000000000000000 <.data..Lubsan_data1>:

                        0: R_390_64     .rodata      <=== source_location.location->file_name
   8:   00 00 00 04             .long   0x00000004   <=== source_location.location->line
   c:   00 00 00 05             .long   0x00000005   <=== source_location.location->column  

                        10: R_390_64    .data..Lubsan_type0   <== source_location->array_type
                        18: R_390_64    .data..Lubsan_type1   <=== source_location->index_type

2e. readelf -p .rodata sample.o

String dump of section '.rodata':
  [     0]  sample.c
  [     a]  arr[101] = %d

  2f. readelf -p .data..Lubsan_type0 sample.o

String dump of section '.data..Lubsan_type0':
[     4]  'int [100]'

  2g. readelf -p .data..Lubsan_type1 sample.o :

[     4]  'int'

Solution 1: Disable ubsan config for patched functions. kernel build can still run with ubsan enabled. Code generated by patched functions wouldnt contain for eg. __ubsan_handle_out_of_bounds

diff --git a/kpatch-build/kpatch-build b/kpatch-build/kpatch-build
index 45e8b14..28daf54 100755
--- a/kpatch-build/kpatch-build
+++ b/kpatch-build/kpatch-build
@@ -989,6 +989,8 @@ if [[ -z "$OOT_MODULE" && ! "$CONFIGFILE" -ef "$KERNEL_SRCDIR"/.config ]] ; then
        cp -f "$CONFIGFILE" "$KERNEL_SRCDIR/.config" || die
 fi

+# Dont compile patched functions with ubsan
+"$KERNEL_SRCDIR"/scripts/config --set-val CONFIG_UBSAN n || die "couldnt disable UBSAN"
 # kernel option checking

 trace_off "reading .config"

Question/Solution 2: As per my understanding, kpatch can deal with:

• modified and new functions,
• addition of new data.
• Can it also support modification to other .data.* ?
  • Considering these References:
  • 303928f ("create-diff-object: ensure no data sections are included")
  • 2982962 ("support forkey and warned special static local vars")
  • patch-author-guide.md: Data structure changes kpatch patches functions, not data. If the original patch involves a change to a data structure, the patch will require some rework, as changes to data structures are not allowed by default.

In the below patch, I avoided correlating the .data.Lubsan_ and this means inclusion of the new .data.Lubsan sections. Considering the fact that .data.Lubsan contains linenumber, filename and datatype info, Is it safe to include the new .data.Lubsan ?

I just ran a simple test on this and new data are reflected, Also, when the array is out of bounds in the patched functions a warning is thrown. unloading points to the old data.

From 87a117d6d2f5b6b5594ce0f5af16f2d7d93ae7ab Mon Sep 17 00:00:00 2001
From: Sumanth Korikkar <sumanthk@linux.ibm.com>
Date: Tue, 21 Feb 2023 15:28:21 +0100
Subject: [PATCH] Dont correlate .data.Lubsan* sections

Signed-off-by: Sumanth Korikkar <sumanthk@linux.ibm.com>
---
 kpatch-build/create-diff-object.c |  6 ++++++
 kpatch-build/kpatch-elf.c         | 12 ++++++++++++
 kpatch-build/kpatch-elf.h         |  1 +
 kpatch-build/lookup.c             |  3 ++-
 4 files changed, 21 insertions(+), 1 deletion(-)

diff --git a/kpatch-build/create-diff-object.c b/kpatch-build/create-diff-object.c
index 707b0a9..014737c 100644
--- a/kpatch-build/create-diff-object.c
+++ b/kpatch-build/create-diff-object.c
@@ -1036,6 +1036,9 @@ static void kpatch_correlate_sections(struct list_head *seclist_orig,
                sec_patched->twin)
                continue;

+           if (is_ubsan_data(sec_orig->name))
+               continue;
+
            if (is_special_static(is_rela_section(sec_orig) ?
                          sec_orig->base->secsym :
                          sec_orig->secsym))
@@ -1072,6 +1075,9 @@ static void kpatch_correlate_symbols(struct list_head *symlist_orig,
                sym_orig->type != sym_patched->type || sym_patched->twin)
                continue;

+           if (is_ubsan_data(sym_orig->name))
+               continue;
+
            if (is_special_static(sym_orig))
                continue;

diff --git a/kpatch-build/kpatch-elf.c b/kpatch-build/kpatch-elf.c
index c7d12ec..f02e5d4 100644
--- a/kpatch-build/kpatch-elf.c
+++ b/kpatch-build/kpatch-elf.c
@@ -587,6 +587,18 @@ bool is_local_sym(struct symbol *sym)
    return sym->bind == STB_LOCAL;
 }

+bool is_ubsan_data(const char *name) {
+   if (!strncmp(name, ".data.rel.local..Lubsan_data", 28) ||
+       !strncmp(name, ".data..Lubsan_type", 18) ||
+       !strncmp(name, ".Lubsan_data", 12) ||
+       !strncmp(name, ".data..Lubsan_data", 18) ||
+       !strncmp(name, ".rela.data..Lubsan_data", 23) ||
+       !strncmp(name, ".rela.data.rel.local..Lubsan_data", 33))
+       return true;
+   else
+       return false;
+}
+
 void print_strtab(char *buf, size_t size)
 {
    size_t i;
diff --git a/kpatch-build/kpatch-elf.h b/kpatch-build/kpatch-elf.h
index cd2900c..d340f1f 100644
--- a/kpatch-build/kpatch-elf.h
+++ b/kpatch-build/kpatch-elf.h
@@ -170,6 +170,7 @@ bool is_null_sym(struct symbol *sym);
 bool is_file_sym(struct symbol *sym);
 bool is_local_func_sym(struct symbol *sym);
 bool is_local_sym(struct symbol *sym);
+bool is_ubsan_data(const char *name);

 void print_strtab(char *buf, size_t size);
 void kpatch_create_shstrtab(struct kpatch_elf *kelf);
diff --git a/kpatch-build/lookup.c b/kpatch-build/lookup.c
index f2596b1..da1c3f8 100644
--- a/kpatch-build/lookup.c
+++ b/kpatch-build/lookup.c
@@ -84,7 +84,8 @@ static bool maybe_discarded_sym(const char *name)
        !strncmp(name, "__func_stack_frame_non_standard_", 32) ||
        strstr(name, "__addressable_") ||
        strstr(name, "__UNIQUE_ID_") ||
-       !strncmp(name, ".L.str", 6))
+       !strncmp(name, ".L.str", 6) ||
+       is_ubsan_data(name))
        return true;

    return false;
-- 
2.38.1

Request for your suggestions and feedback.

Thank you, Sumanth

[jpoimboe]

Solution 2 sounds fine to me.

[jpoimboe]

Merged with #1332