Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix linking problems/warnings in ubsan #7198

Merged
merged 4 commits into from
Jan 8, 2025
Merged

Fix linking problems/warnings in ubsan #7198

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
b9386f0
core: fix undefined reference to ubsan function
alexhoppus Dec 26, 2024
c1846b0
core: fix gcc warnings in ubsan
alexhoppus Dec 27, 2024
64b8bf7
core: fix error in ubsan_builtin_unreachable naming
alexhoppus Dec 27, 2024
035a02c
core: refactor ubsan panic code
alexhoppus Dec 27, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
201 changes: 98 additions & 103 deletions core/kernel/ubsan.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,44 +63,42 @@ struct nonnull_arg_data {
struct source_location loc;
};

struct invalid_builtin_data {
struct source_location loc;
unsigned char kind;
};

/*
* When compiling with -fsanitize=undefined the compiler expects functions
* with the following signatures. The functions are never called directly,
* only when undefined behavior is detected in instrumented code.
*/
void __ubsan_handle_type_mismatch(struct type_mismatch_data *data,
unsigned long ptr);
void __ubsan_handle_type_mismatch_v1(struct type_mismatch_data *data,
unsigned long ptr);
void __ubsan_handle_add_overflow(struct overflow_data *data,
unsigned long lhs, unsigned long rhs);
void __ubsan_handle_sub_overflow(struct overflow_data *data,
unsigned long lhs, unsigned long rhs);
void __ubsan_handle_mul_overflow(struct overflow_data *data,
unsigned long lhs, unsigned long rhs);
void __ubsan_handle_negate_overflow(struct overflow_data *data,
unsigned long old_val);
void __ubsan_handle_divrem_overflow(struct overflow_data *data,
unsigned long lhs, unsigned long rhs);
void __ubsan_handle_pointer_overflow(struct overflow_data *data,
unsigned long lhs, unsigned long rhs);
void __ubsan_handle_shift_out_of_bounds(struct shift_out_of_bounds_data *data,
unsigned long lhs, unsigned long rhs);
void __ubsan_handle_out_of_bounds(struct out_of_bounds_data *data,
unsigned long idx);
void __ubsan_handle_unreachable(struct unreachable_data *data);
void __ubsan_handle_missing_return(struct unreachable_data *data);
void __ubsan_handle_vla_bound_not_positive(struct vla_bound_data *data,
unsigned long bound);
void __ubsan_handle_load_invalid_value(struct invalid_value_data *data,
unsigned long val);
void __ubsan_handle_nonnull_arg(struct nonnull_arg_data *data
void __ubsan_handle_type_mismatch_v1(void *data_, void *ptr);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How did you find the proper declaration of this and the other __ubsan_handle_ functions?
Do you have a link?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi,

There are at least 3 ways

  1. gcc gives warning during compilation and shows correct prototype at the same time. So one can rely on this gcc warning messages to figure out correct function signature. The initial idea behind this was to fix warnings.
    For example:
core/kernel/ubsan.c:78:6: warning: conflicting types for built-in function ‘__ubsan_handle_type_mismatch_v1’; expected ‘void(void *, void *)’ [-Wbuiltin-declaration-mismatch]
   78 | void __ubsan_handle_type_mismatch_v1(void *data_, unsigned long ptr);
  1. one can refer to linux kernel implementation
  2. look into gcc . I hope I gave a link to correct place

void __ubsan_handle_add_overflow(void *data_, void *lhs, void *rhs);
void __ubsan_handle_sub_overflow(void *data_, void *lhs, void *rhs);
void __ubsan_handle_mul_overflow(void *data_, void *lhs, void *rhs);
void __ubsan_handle_negate_overflow(void *data_, void *old_val);
void __ubsan_handle_divrem_overflow(void *data_, void *lhs, void *rhs);
void __ubsan_handle_pointer_overflow(void *data_, void *lhs, void *rhs);
void __ubsan_handle_shift_out_of_bounds(void *data_, void *lhs, void *rhs);
void __ubsan_handle_out_of_bounds(void *data_, void *idx);
void __ubsan_handle_builtin_unreachable(void *data_);
void __ubsan_handle_missing_return(void *data_);
void __ubsan_handle_vla_bound_not_positive(void *data_, void *bound);
void __ubsan_handle_load_invalid_value(void *data_, void *val);
void __ubsan_handle_nonnull_arg(void *data_
#if __GCC_VERSION < 60000
, size_t arg_no
#endif
);
void __ubsan_handle_invalid_builtin(void *data_);

static void print_loc(const char *func, struct source_location *loc)
static bool ubsan_panic = true;

static void ubsan_handle_error(const char *func, struct source_location *loc,
bool should_panic)
{
const char *f = func;
const char func_prefix[] = "__ubsan_handle";
Expand All @@ -110,133 +108,130 @@ static void print_loc(const char *func, struct source_location *loc)

EMSG_RAW("Undefined behavior %s at %s:%" PRIu32 " col %" PRIu32,
f, loc->file_name, loc->line, loc->column);
}


static volatile bool ubsan_panic = true;
if (should_panic)
panic();
}

void __ubsan_handle_type_mismatch(struct type_mismatch_data *data,
unsigned long ptr __unused)
{
print_loc(__func__, &data->loc);
if (ubsan_panic)
panic();
ubsan_handle_error(__func__, &data->loc, ubsan_panic);
}

void __ubsan_handle_type_mismatch_v1(struct type_mismatch_data *data,
unsigned long ptr __unused)
void __ubsan_handle_type_mismatch_v1(void *data_, void *ptr __unused)
{
print_loc(__func__, &data->loc);
if (ubsan_panic)
panic();
struct type_mismatch_data *data = data_;

ubsan_handle_error(__func__, &data->loc, ubsan_panic);
}

void __ubsan_handle_add_overflow(struct overflow_data *data,
unsigned long lhs __unused,
unsigned long rhs __unused)
void __ubsan_handle_add_overflow(void *data_, void *lhs __unused,
void *rhs __unused)
{
print_loc(__func__, &data->loc);
if (ubsan_panic)
panic();
struct overflow_data *data = data_;

ubsan_handle_error(__func__, &data->loc, ubsan_panic);
}

void __ubsan_handle_sub_overflow(struct overflow_data *data,
unsigned long lhs __unused,
unsigned long rhs __unused)
void __ubsan_handle_sub_overflow(void *data_, void *lhs __unused,
void *rhs __unused)
{
print_loc(__func__, &data->loc);
if (ubsan_panic)
panic();
struct overflow_data *data = data_;

ubsan_handle_error(__func__, &data->loc, ubsan_panic);
}

void __ubsan_handle_mul_overflow(struct overflow_data *data,
unsigned long lhs __unused,
unsigned long rhs __unused)
void __ubsan_handle_mul_overflow(void *data_, void *lhs __unused,
void *rhs __unused)
{
print_loc(__func__, &data->loc);
if (ubsan_panic)
panic();
struct overflow_data *data = data_;

ubsan_handle_error(__func__, &data->loc, ubsan_panic);
}

void __ubsan_handle_negate_overflow(struct overflow_data *data,
unsigned long old_val __unused)
void __ubsan_handle_negate_overflow(void *data_, void *old_val __unused)
{
print_loc(__func__, &data->loc);
if (ubsan_panic)
panic();
struct overflow_data *data = data_;

ubsan_handle_error(__func__, &data->loc, ubsan_panic);
}

void __ubsan_handle_divrem_overflow(struct overflow_data *data,
unsigned long lhs __unused,
unsigned long rhs __unused)
void __ubsan_handle_divrem_overflow(void *data_, void *lhs __unused,
void *rhs __unused)
{
print_loc(__func__, &data->loc);
if (ubsan_panic)
panic();
struct overflow_data *data = data_;

ubsan_handle_error(__func__, &data->loc, ubsan_panic);
}

void __ubsan_handle_pointer_overflow(struct overflow_data *data,
unsigned long lhs __unused,
unsigned long rhs __unused)
void __ubsan_handle_pointer_overflow(void *data_, void *lhs __unused,
void *rhs __unused)
{
print_loc(__func__, &data->loc);
if (ubsan_panic)
panic();
struct overflow_data *data = data_;

ubsan_handle_error(__func__, &data->loc, ubsan_panic);
}

void __ubsan_handle_shift_out_of_bounds(struct shift_out_of_bounds_data *data,
unsigned long lhs __unused,
unsigned long rhs __unused)
void __ubsan_handle_shift_out_of_bounds(void *data_, void *lhs __unused,
void *rhs __unused)
{
print_loc(__func__, &data->loc);
if (ubsan_panic)
panic();
struct shift_out_of_bounds_data *data = data_;

ubsan_handle_error(__func__, &data->loc, ubsan_panic);
}

void __ubsan_handle_out_of_bounds(struct out_of_bounds_data *data,
unsigned long idx __unused)
void __ubsan_handle_out_of_bounds(void *data_, void *idx __unused)
{
print_loc(__func__, &data->loc);
if (ubsan_panic)
panic();
struct out_of_bounds_data *data = data_;

ubsan_handle_error(__func__, &data->loc, ubsan_panic);
}

void __ubsan_handle_unreachable(struct unreachable_data *data)
void __ubsan_handle_builtin_unreachable(void *data_)
{
print_loc(__func__, &data->loc);
if (ubsan_panic)
panic();
struct unreachable_data *data = data_;

ubsan_handle_error(__func__, &data->loc, false);
panic();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't this result in a warning?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi,

I think the opposite results in a warning, since this function marked with attribute noreturn. So if it doesn't call explicitly any other noreturn function or doesn't end with infinite loop I get the warning.

core/kernel/ubsan.c: In function ‘__ubsan_handle_builtin_unreachable’:
core/kernel/ubsan.c:188:1: warning: ‘noreturn’ function does return
  188 | }

That is the reason why I treated noreturn functions in a little bit special way here.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That means that this function has an implicit noreturn attribute. How about adding it explicitly to be clear?

Copy link
Contributor Author

@alexhoppus alexhoppus Jan 2, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That means that this function has an implicit noreturn attribute. How about adding it explicitly to be clear?

That was exactly my thought and I tried to do this. But for some weird reason I get the warning message in this case

core/kernel/ubsan.c:182:1: warning: ignoring attribute ‘noreturn’ in declaration of a built-in function ‘__ubsan_handle_builtin_unreachable’ because it conflicts with attribute ‘const’ [-Wattributes]
  182 | {
      | ^

After some research I found this. So I can mark it as noreturn, but we will see this warning. If you are okay, then I will do that.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair enough, it seems like a forever bug in GCC, let's keep it as it is.

}

void __noreturn __ubsan_handle_missing_return(struct unreachable_data *data)
void __noreturn __ubsan_handle_missing_return(void *data_)
{
print_loc(__func__, &data->loc);
struct unreachable_data *data = data_;

ubsan_handle_error(__func__, &data->loc, false);
panic();
}

void __ubsan_handle_vla_bound_not_positive(struct vla_bound_data *data,
unsigned long bound __unused)
void __ubsan_handle_vla_bound_not_positive(void *data_, void *bound __unused)
{
print_loc(__func__, &data->loc);
if (ubsan_panic)
panic();
struct vla_bound_data *data = data_;

ubsan_handle_error(__func__, &data->loc, ubsan_panic);
}

void __ubsan_handle_load_invalid_value(struct invalid_value_data *data,
unsigned long val __unused)
void __ubsan_handle_load_invalid_value(void *data_, void *val __unused)
{
print_loc(__func__, &data->loc);
if (ubsan_panic)
panic();
struct invalid_value_data *data = data_;

ubsan_handle_error(__func__, &data->loc, ubsan_panic);
}

void __ubsan_handle_nonnull_arg(struct nonnull_arg_data *data
void __ubsan_handle_nonnull_arg(void *data_
#if __GCC_VERSION < 60000
, size_t arg_no __unused
#endif
)
{
print_loc(__func__, &data->loc);
if (ubsan_panic)
panic();
struct nonnull_arg_data *data = data_;

ubsan_handle_error(__func__, &data->loc, ubsan_panic);
}

void __ubsan_handle_invalid_builtin(void *data_)
{
struct invalid_builtin_data *data = data_;

ubsan_handle_error(__func__, &data->loc, ubsan_panic);
}
Loading