On Mon, May 13, 2024 at 02:46:32PM -0600, Jeff Law wrote:
>
>
> On 5/13/24 1:48 PM, Qing Zhao wrote:
> > -Warray-bounds is an important option to enable linux kernal to keep
> > the array out-of-bound errors out of the source tree.
> >
> > However, due to the false positive warnings reported in PR109071
> > (-Warray-bounds false positive warnings due to code duplication from
> > jump threading), -Warray-bounds=1 cannot be added on by default.
> >
> > Although it's impossible to elinimate all the false positive warnings
> > from -Warray-bounds=1 (See PR104355 Misleading -Warray-bounds
> > documentation says "always out of bounds"), we should minimize the
> > false positive warnings in -Warray-bounds=1.
> >
> > The root reason for the false positive warnings reported in PR109071 is:
> >
> > When the thread jump optimization tries to reduce the # of branches
> > inside the routine, sometimes it needs to duplicate the code and
> > split into two conditional pathes. for example:
> >
> > The original code:
> >
> > void sparx5_set (int * ptr, struct nums * sg, int index)
> > {
> > if (index >= 4)
> > warn ();
> > *ptr = 0;
> > *val = sg->vals[index];
> > if (index >= 4)
> > warn ();
> > *ptr = *val;
> >
> > return;
> > }
> >
> > With the thread jump, the above becomes:
> >
> > void sparx5_set (int * ptr, struct nums * sg, int index)
> > {
> > if (index >= 4)
> > {
> > warn ();
> > *ptr = 0; // Code duplications since "warn" does return;
> > *val = sg->vals[index]; // same this line.
> > // In this path, since it's under the condition
> > // "index >= 4", the compiler knows the value
> > // of "index" is larger then 4, therefore the
> > // out-of-bound warning.
> > warn ();
> > }
> > else
> > {
> > *ptr = 0;
> > *val = sg->vals[index];
> > }
> > *ptr = *val;
> > return;
> > }
> >
> > We can see, after the thread jump optimization, the # of branches inside
> > the routine "sparx5_set" is reduced from 2 to 1, however, due to the
> > code duplication (which is needed for the correctness of the code), we
> > got a false positive out-of-bound warning.
> >
> > In order to eliminate such false positive out-of-bound warning,
> >
> > A. Add one more flag for GIMPLE: is_splitted.
> > B. During the thread jump optimization, when the basic blocks are
> > duplicated, mark all the STMTs inside the original and duplicated
> > basic blocks as "is_splitted";
> > C. Inside the array bound checker, add the following new heuristic:
> >
> > If
> > 1. the stmt is duplicated and splitted into two conditional paths;
> > + 2. the warning level < 2;
> > + 3. the current block is not dominating the exit block
> > Then not report the warning.
> >
> > The false positive warnings are moved from -Warray-bounds=1 to
> > -Warray-bounds=2 now.
> >
> > Bootstrapped and regression tested on both x86 and aarch64. adjusted
> > -Warray-bounds-61.c due to the false positive warnings.
> >
> > Let me know if you have any comments and suggestions.
> This sounds horribly wrong. In the code above, the warning is correct.
It's not sensible from a user's perspective.
If this doesn't warn:
void sparx5_set (int * ptr, struct nums * sg, int index)
{
*ptr = 0;
*val = sg->vals[index];
*ptr = *val;
}
... because the value range tracking of "index" spans [INT_MIN,INT_MAX],
and warnings based on the value range are silenced if they haven't been
clamped at all. (Otherwise warnings would be produced everywhere: only
when a limited set of values is known is it useful to produce a warning.)
But it makes no sense to warn about:
void sparx5_set (int * ptr, struct nums * sg, int index)
{
if (index >= 4)
warn ();
*ptr = 0;
*val = sg->vals[index];
if (index >= 4)
warn ();
*ptr = *val;
}
Because at "*val = sg->vals[index];" the actual value range tracking for
index is _still_ [INT_MIN,INT_MAX]. (Only within the "then" side of the
"if" statements is the range tracking [4,INT_MAX].)
However, in the case where jump threading has split the execution flow
and produced a copy of "*val = sg->vals[index];" where the value range
tracking for "index" is now [4,INT_MAX], is the warning valid. But it
is only for that instance. Reporting it for effectively both (there is
only 1 source line for the array indexing) is misleading because there
is nothing the user can do about it -- the compiler created the copy and
then noticed it had a range it could apply to that array index.
This situation makes -Warray-bounds unusable for the Linux kernel (we
cannot have false positives says BDFL), but we'd *really* like to have
it enabled since it usually finds real bugs. But these false positives
can't be fixed on our end. :( So, moving them to -Warray-bounds=2 makes
sense as that's the level documented to have potential false positives.
-Kees
--
Kees Cook
