In COVERAGE=y but DEBUG=n builds (observed by randconfig testing) gcc12
takes issue with the subtraction of 1 from __stop___pre_ex_table[],
considering this an out of bounds access. Not being able to know that
the symbol actually marks the end of an array, the compiler is kind of
right with this diagnosis. Move the subtraction into the function.
Reported-by: Anthony PERARD <anthony.perard@citrix.com>
Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
To keep things simple, I'm merely calculating "last" as a local variable
now, rather than replacing its uses by suitable ones of "end". In the
longer run it may become necessary to actually go this 2nd step, as in
principle the compiler could inline the function and then still spot the
same issue. However, while the subtraction of 1 can likely be avoided by
suitable other adjustments, "last - first" cannot easily be. Yet that's
also an offense, in that it's calculating the difference between pointers
into distinct objects.
--- a/xen/arch/x86/extable.c
+++ b/xen/arch/x86/extable.c
@@ -64,9 +64,10 @@ void __init sort_exception_tables(void)
static unsigned long
search_one_extable(const struct exception_table_entry *first,
- const struct exception_table_entry *last,
+ const struct exception_table_entry *end,
unsigned long value)
{
+ const struct exception_table_entry *last = end - 1;
const struct exception_table_entry *mid;
long diff;
@@ -91,7 +92,7 @@ search_exception_table(const struct cpu_
unsigned long stub = this_cpu(stubs.addr);
if ( region && region->ex )
- return search_one_extable(region->ex, region->ex_end - 1, regs->rip);
+ return search_one_extable(region->ex, region->ex_end, regs->rip);
if ( regs->rip >= stub + STUB_BUF_SIZE / 2 &&
regs->rip < stub + STUB_BUF_SIZE &&
@@ -102,7 +103,7 @@ search_exception_table(const struct cpu_
region = find_text_region(retptr);
retptr = region && region->ex
- ? search_one_extable(region->ex, region->ex_end - 1, retptr)
+ ? search_one_extable(region->ex, region->ex_end, retptr)
: 0;
if ( retptr )
{
@@ -198,7 +199,7 @@ search_pre_exception_table(struct cpu_us
{
unsigned long addr = regs->rip;
unsigned long fixup = search_one_extable(
- __start___pre_ex_table, __stop___pre_ex_table-1, addr);
+ __start___pre_ex_table, __stop___pre_ex_table, addr);
if ( fixup )
{
dprintk(XENLOG_INFO, "Pre-exception: %p -> %p\n", _p(addr), _p(fixup));
On 22/02/2023 10:22 am, Jan Beulich wrote: > In COVERAGE=y but DEBUG=n builds (observed by randconfig testing) gcc12 > takes issue with the subtraction of 1 from __stop___pre_ex_table[], > considering this an out of bounds access. Not being able to know that > the symbol actually marks the end of an array, the compiler is kind of > right with this diagnosis. Move the subtraction into the function. > > Reported-by: Anthony PERARD <anthony.perard@citrix.com> > Signed-off-by: Jan Beulich <jbeulich@suse.com> > --- > To keep things simple, I'm merely calculating "last" as a local variable > now, rather than replacing its uses by suitable ones of "end". In the > longer run it may become necessary to actually go this 2nd step, as in > principle the compiler could inline the function and then still spot the > same issue. However, while the subtraction of 1 can likely be avoided by > suitable other adjustments, "last - first" cannot easily be. Yet that's > also an offense, in that it's calculating the difference between pointers > into distinct objects. All of these bugs are ultimately because gcc doesn't know that these two labels are the bounds of a single array, and not separate objects. There is no possible at all to get rid of the "last - first" calculation - this is a binary search through an array. But it's also not going to actually stop working, because this is the common (and documented) way of getting linker symbols into C. For the patch, Acked-by: Andrew Cooper <andrew.cooper3@citrix.com> but if it were me, I'd have gone one step further and made search_one_extable() into a more normal looking binary search.
On 22.02.2023 12:15, Andrew Cooper wrote: > On 22/02/2023 10:22 am, Jan Beulich wrote: >> In COVERAGE=y but DEBUG=n builds (observed by randconfig testing) gcc12 >> takes issue with the subtraction of 1 from __stop___pre_ex_table[], >> considering this an out of bounds access. Not being able to know that >> the symbol actually marks the end of an array, the compiler is kind of >> right with this diagnosis. Move the subtraction into the function. >> >> Reported-by: Anthony PERARD <anthony.perard@citrix.com> >> Signed-off-by: Jan Beulich <jbeulich@suse.com> >> --- >> To keep things simple, I'm merely calculating "last" as a local variable >> now, rather than replacing its uses by suitable ones of "end". In the >> longer run it may become necessary to actually go this 2nd step, as in >> principle the compiler could inline the function and then still spot the >> same issue. However, while the subtraction of 1 can likely be avoided by >> suitable other adjustments, "last - first" cannot easily be. Yet that's >> also an offense, in that it's calculating the difference between pointers >> into distinct objects. > > All of these bugs are ultimately because gcc doesn't know that these two > labels are the bounds of a single array, and not separate objects. > > There is no possible at all to get rid of the "last - first" calculation > - this is a binary search through an array. But it's also not going to > actually stop working, because this is the common (and documented) way > of getting linker symbols into C. I disagree with the "impossible" aspect: Rather than surfacing start/end symbols, a start/size pair would eliminate all such concerns. Recall my attempt to make use of .startof. and .sizeof. assembly operators, which sadly Clang doesn't support? (These aren't properly usable here, because there's no separate .extable section in the final binary, so the remark is merely to support the point I'm trying to make.) I'm also unsure of that supposedly documented way actually covering symbols at the end of objects. If that really was meant to be like that, then why would Misra make an issue of it? > For the patch, Acked-by: Andrew Cooper <andrew.cooper3@citrix.com> but Thanks. > if it were me, I'd have gone one step further and made > search_one_extable() into a more normal looking binary search. Well, I kind of inferred that from your reply on the original thread, but I have to admit that it's not really clear to me why things would end up "more normal looking" if the upper bound is exclusive rather than inclusive. Jan
© 2016 - 2024 Red Hat, Inc.