| Message ID | 20171018074532.33574-1-roger.pau@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
>>> On 18.10.17 at 09:45, <roger.pau@citrix.com> wrote: > +void __ubsan_handle_pointer_overflow(struct pointer_overflow_data *data, > + unsigned long base, unsigned long result) > +{ > + unsigned long flags; > + > + if (suppress_report(&data->location)) > + return; > + > + ubsan_prologue(&data->location, &flags); > + > + if (((long)base >= 0) == ((long)result >= 0)) > + pr_err("pointer operation %s %p to %p\n", > + base > result ? "underflowed" : "overflowed", > + (void *)base, (void *)result); > + else > + pr_err("pointer index expression with base %p overflowed to %p\n", > + (void *)base, (void *)result); Would you mind explaining the difference between if and else branches? (I do realize I should have asked this on v1 already, but I didn't pay enough attention.) Whatever the idea behind this, it should probably be explained in a comment, as it looks to be heuristic. Jan
On Wed, Oct 18, 2017 at 03:23:20AM -0600, Jan Beulich wrote: > >>> On 18.10.17 at 09:45, <roger.pau@citrix.com> wrote: > > +void __ubsan_handle_pointer_overflow(struct pointer_overflow_data *data, > > + unsigned long base, unsigned long result) > > +{ > > + unsigned long flags; > > + > > + if (suppress_report(&data->location)) > > + return; > > + > > + ubsan_prologue(&data->location, &flags); > > + > > + if (((long)base >= 0) == ((long)result >= 0)) > > + pr_err("pointer operation %s %p to %p\n", > > + base > result ? "underflowed" : "overflowed", > > + (void *)base, (void *)result); > > + else > > + pr_err("pointer index expression with base %p overflowed to %p\n", > > + (void *)base, (void *)result); > > Would you mind explaining the difference between if and else > branches? (I do realize I should have asked this on v1 already, > but I didn't pay enough attention.) Whatever the idea behind > this, it should probably be explained in a comment, as it looks > to be heuristic. The upstream commit is: https://github.com/llvm-mirror/compiler-rt/commit/079b7657767dcc0fb284225c277d2b9ce73e423b However it's lacking a proper commit message. It seems to me like it's there to detect addition of signed + unsigned values when an overflow happens, but I don't really see it's value rather than just using the first message. Thanks, Roger.
>>> On 18.10.17 at 11:42, <roger.pau@citrix.com> wrote: > On Wed, Oct 18, 2017 at 03:23:20AM -0600, Jan Beulich wrote: >> >>> On 18.10.17 at 09:45, <roger.pau@citrix.com> wrote: >> > +void __ubsan_handle_pointer_overflow(struct pointer_overflow_data *data, >> > + unsigned long base, unsigned long result) >> > +{ >> > + unsigned long flags; >> > + >> > + if (suppress_report(&data->location)) >> > + return; >> > + >> > + ubsan_prologue(&data->location, &flags); >> > + >> > + if (((long)base >= 0) == ((long)result >= 0)) >> > + pr_err("pointer operation %s %p to %p\n", >> > + base > result ? "underflowed" : "overflowed", >> > + (void *)base, (void *)result); >> > + else >> > + pr_err("pointer index expression with base %p overflowed to %p\n", >> > + (void *)base, (void *)result); >> >> Would you mind explaining the difference between if and else >> branches? (I do realize I should have asked this on v1 already, >> but I didn't pay enough attention.) Whatever the idea behind >> this, it should probably be explained in a comment, as it looks >> to be heuristic. > > The upstream commit is: > > https://github.com/llvm-mirror/compiler-rt/commit/079b7657767dcc0fb284225c277d > 2b9ce73e423b > > However it's lacking a proper commit message. It seems to me like it's > there to detect addition of signed + unsigned values when an overflow > happens, but I don't really see it's value rather than just using the > first message. Right - me too. I'd therefore like to simply drop the "if" and the "else" branch (likely easily done while committing), and then the change is Acked-by: Jan Beulich <jbeulich@suse.com> Jan
On Wed, Oct 18, 2017 at 03:53:37AM -0600, Jan Beulich wrote: > >>> On 18.10.17 at 11:42, <roger.pau@citrix.com> wrote: > > On Wed, Oct 18, 2017 at 03:23:20AM -0600, Jan Beulich wrote: > >> >>> On 18.10.17 at 09:45, <roger.pau@citrix.com> wrote: > >> > +void __ubsan_handle_pointer_overflow(struct pointer_overflow_data *data, > >> > + unsigned long base, unsigned long result) > >> > +{ > >> > + unsigned long flags; > >> > + > >> > + if (suppress_report(&data->location)) > >> > + return; > >> > + > >> > + ubsan_prologue(&data->location, &flags); > >> > + > >> > + if (((long)base >= 0) == ((long)result >= 0)) > >> > + pr_err("pointer operation %s %p to %p\n", > >> > + base > result ? "underflowed" : "overflowed", > >> > + (void *)base, (void *)result); > >> > + else > >> > + pr_err("pointer index expression with base %p overflowed to %p\n", > >> > + (void *)base, (void *)result); > >> > >> Would you mind explaining the difference between if and else > >> branches? (I do realize I should have asked this on v1 already, > >> but I didn't pay enough attention.) Whatever the idea behind > >> this, it should probably be explained in a comment, as it looks > >> to be heuristic. > > > > The upstream commit is: > > > > https://github.com/llvm-mirror/compiler-rt/commit/079b7657767dcc0fb284225c277d > > 2b9ce73e423b > > > > However it's lacking a proper commit message. It seems to me like it's > > there to detect addition of signed + unsigned values when an overflow > > happens, but I don't really see it's value rather than just using the > > first message. > > Right - me too. I'd therefore like to simply drop the "if" and the "else" > branch (likely easily done while committing), and then the change is > Acked-by: Jan Beulich <jbeulich@suse.com> Yes, feel free to drop the if/else and just keep the first error message. Thanks, Roger.
On Wed, Oct 18, 2017 at 08:45:32AM +0100, Roger Pau Monne wrote: > clang 5.0 changed the layout of the type_mismatch_data structure and > introduced __ubsan_handle_type_mismatch_v1 and > __ubsan_handle_pointer_overflow. > > This commit adds support for the new structure layout, adds the > missing handlers and the new types for type_check_kinds. > > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> With existing comments addressed: Acked-by: Wei Liu <wei.liu2@citrix.com>
Hi Roger, On 10/18/2017 08:45 AM, Roger Pau Monne wrote: > clang 5.0 changed the layout of the type_mismatch_data structure and > introduced __ubsan_handle_type_mismatch_v1 and > __ubsan_handle_pointer_overflow. > > This commit adds support for the new structure layout, adds the > missing handlers and the new types for type_check_kinds. > > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> > --- > Cc: Andrew Cooper <andrew.cooper3@citrix.com> > Cc: George Dunlap <George.Dunlap@eu.citrix.com> > Cc: Ian Jackson <ian.jackson@eu.citrix.com> > Cc: Jan Beulich <jbeulich@suse.com> > Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> > Cc: Stefano Stabellini <sstabellini@kernel.org> > Cc: Tim Deegan <tim@xen.org> > Cc: Wei Liu <wei.liu2@citrix.com> > Cc: Julien Grall <julien.grall@arm.com> > --- > ubsan is an optional feature, not enabled by default and not designed > to be used by production systems. Since this change only touches ubsan > code and it's a bugfix in order for clang to work, I argue it should > be merged into 4.10. I agree here: Release-acked-by: Julien Grall <julien.grall@linaro.org> Cheers,
diff --git a/xen/common/ubsan/ubsan.c b/xen/common/ubsan/ubsan.c index fbe568562a..2eaa403691 100644 --- a/xen/common/ubsan/ubsan.c +++ b/xen/common/ubsan/ubsan.c @@ -33,7 +33,10 @@ const char *type_check_kinds[] = { "member call on", "constructor call on", "downcast of", - "downcast of" + "downcast of", + "upcast of", + "cast to virtual base of", + "_Nonnull binding to", }; #define REPORTED_BIT 31 @@ -333,6 +336,26 @@ void __ubsan_handle_type_mismatch(struct type_mismatch_data *data, } EXPORT_SYMBOL(__ubsan_handle_type_mismatch); +void __ubsan_handle_type_mismatch_v1(struct type_mismatch_data_v1 *data, + unsigned long ptr) +{ + struct type_mismatch_data d = { + .location = data->location, + .type = data->type, + .alignment = 1ul << data->log_alignment, + .type_check_kind = data->type_check_kind, + }; + + /* + * NB: do the check with data->location, d->location is just a local + * copy and the modifications will be lost. + */ + if (suppress_report(&data->location)) + return; + + __ubsan_handle_type_mismatch(&d, ptr); +} + void __ubsan_handle_nonnull_arg(struct nonnull_arg_data *data) { unsigned long flags; @@ -478,3 +501,24 @@ void __ubsan_handle_load_invalid_value(struct invalid_value_data *data, ubsan_epilogue(&flags); } EXPORT_SYMBOL(__ubsan_handle_load_invalid_value); + +void __ubsan_handle_pointer_overflow(struct pointer_overflow_data *data, + unsigned long base, unsigned long result) +{ + unsigned long flags; + + if (suppress_report(&data->location)) + return; + + ubsan_prologue(&data->location, &flags); + + if (((long)base >= 0) == ((long)result >= 0)) + pr_err("pointer operation %s %p to %p\n", + base > result ? "underflowed" : "overflowed", + (void *)base, (void *)result); + else + pr_err("pointer index expression with base %p overflowed to %p\n", + (void *)base, (void *)result); + + ubsan_epilogue(&flags); +} diff --git a/xen/common/ubsan/ubsan.h b/xen/common/ubsan/ubsan.h index b2d18d4a53..2710cd423e 100644 --- a/xen/common/ubsan/ubsan.h +++ b/xen/common/ubsan/ubsan.h @@ -36,6 +36,13 @@ struct type_mismatch_data { unsigned char type_check_kind; }; +struct type_mismatch_data_v1 { + struct source_location location; + struct type_descriptor *type; + unsigned char log_alignment; + unsigned char type_check_kind; +}; + struct nonnull_arg_data { struct source_location location; struct source_location attr_location; @@ -73,6 +80,10 @@ struct invalid_value_data { struct type_descriptor *type; }; +struct pointer_overflow_data { + struct source_location location; +}; + #if defined(CONFIG_ARCH_SUPPORTS_INT128) && defined(__SIZEOF_INT128__) typedef __int128 s_max; typedef unsigned __int128 u_max;
clang 5.0 changed the layout of the type_mismatch_data structure and introduced __ubsan_handle_type_mismatch_v1 and __ubsan_handle_pointer_overflow. This commit adds support for the new structure layout, adds the missing handlers and the new types for type_check_kinds. Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> --- Cc: Andrew Cooper <andrew.cooper3@citrix.com> Cc: George Dunlap <George.Dunlap@eu.citrix.com> Cc: Ian Jackson <ian.jackson@eu.citrix.com> Cc: Jan Beulich <jbeulich@suse.com> Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> Cc: Stefano Stabellini <sstabellini@kernel.org> Cc: Tim Deegan <tim@xen.org> Cc: Wei Liu <wei.liu2@citrix.com> Cc: Julien Grall <julien.grall@arm.com> --- ubsan is an optional feature, not enabled by default and not designed to be used by production systems. Since this change only touches ubsan code and it's a bugfix in order for clang to work, I argue it should be merged into 4.10. --- Changes since v1: - Replace message in __ubsan_handle_pointer_overflow. - Add a suppress_report check in __ubsan_handle_type_mismatch_v1. --- xen/common/ubsan/ubsan.c | 46 +++++++++++++++++++++++++++++++++++++++++++++- xen/common/ubsan/ubsan.h | 11 +++++++++++ 2 files changed, 56 insertions(+), 1 deletion(-)