[Bug analyzer/112792] New: -Wanalyzer-out-of-bounds seen on Linux kernel with certain unions

[Bug analyzer/112792] New: -Wanalyzer-out-of-bounds seen on Linux kernel with certain unions

[dmalcolm at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=112792

            Bug ID: 112792
           Summary: -Wanalyzer-out-of-bounds seen on Linux kernel with
                    certain unions
           Product: gcc
           Version: unknown
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: analyzer
          Assignee: dmalcolm at gcc dot gnu.org
          Reporter: dmalcolm at gcc dot gnu.org
            Blocks: 106358
  Target Milestone: ---

VVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVVV
typedef unsigned int u32;

union msix_perm {
  struct {
    u32 rsvd2 : 8;
    u32 pasid : 20;
  };
  u32 bits;
} __attribute__((__packed__));

union msix_perm mperm;

void idxd_device_set_perm_entry(u32 pasid) {
  mperm.pasid = pasid;
}
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

False positive with -fanalyzer:
t.c: In function ‘idxd_device_set_perm_entry’:
t.c:14:15: warning: buffer overflow [CWE-787] [-Wanalyzer-out-of-bounds]
   14 |   mperm.pasid = pasid;
      |   ~~~~~~~~~~~~^~~~~~~
  event 1
    |
    |   11 | union msix_perm mperm;
    |      |                 ^~~~~
    |      |                 |
    |      |                 (1) capacity: 4 bytes
    |
    +--> ‘idxd_device_set_perm_entry’: event 2
           |
           |   14 |   mperm.pasid = pasid;
           |      |   ~~~~~~~~~~~~^~~~~~~
           |      |               |
           |      |               (2) out-of-bounds write at byte 4 but ‘mperm’
ends at byte 4
           |
t.c:14:15: note: write of 1 byte to beyond the end of ‘mperm’

                     ┌─────────────────────────────────────────┐
                     │    write from ‘pasid’ (type: ‘u32’)     │
                     └─────────────────────────────────────────┘
                                          │
                                          │
                                          v
  ┌──────────────────────────────────────────────────────────────────────┐
  │                  ‘mperm’ (type: ‘union msix_perm’)                   │
  └──────────────────────────────────────────────────────────────────────┘
  ├──────────────────────────────────┬───────────────────────────────────┤
                                     │
                            ╭────────┴────────╮
                            │capacity: 4 bytes│
                            ╰─────────────────╯




Affects trunk: https://godbolt.org/z/oWoY7j6eY
Affects 13.2:  https://godbolt.org/z/vzdEbq6E1

(reduced from drivers/dma/idxd/device.c)


Referenced Bugs:

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=106358
[Bug 106358] [meta-bug] tracker bug for building the Linux kernel with
-fanalyzer

[Bug analyzer/112792] -Wanalyzer-out-of-bounds false positives seen on Linux kernel with certain unions

[dmalcolm at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=112792

David Malcolm <dmalcolm at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |ASSIGNED
     Ever confirmed|0                           |1
   Last reconfirmed|                            |2023-12-15

--- Comment #1 from David Malcolm <dmalcolm at gcc dot gnu.org> ---
Am testing a fix.

[Bug analyzer/112792] -Wanalyzer-out-of-bounds false positives seen on Linux kernel with certain unions

[cvs-commit at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=112792

--- Comment #2 from GCC Commits <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by David Malcolm <dmalcolm@gcc.gnu.org>:

https://gcc.gnu.org/g:5f1bed2a7af828103ca23a3546466a23e8dd2f30

commit r14-6622-g5f1bed2a7af828103ca23a3546466a23e8dd2f30
Author: David Malcolm <dmalcolm@redhat.com>
Date:   Sat Dec 16 09:03:16 2023 -0500

    analyzer: use bit-level granularity for concrete bounds-checking [PR112792]

    PR analyzer/112792 reports false positives from -fanalyzer's
    bounds-checking on certain packed structs containing bitfields e.g.
    in the Linux kernel's drivers/dma/idxd/device.c:

    union msix_perm {
      struct {
        u32 rsvd2 : 8;
        u32 pasid : 20;
      };
      u32 bits;
    } __attribute__((__packed__));

    The root cause is that the bounds-checking is done using byte offsets
    and ranges; in the above, an access of "pasid" is treated as a 32-bit
    access starting one byte inside the union, thus accessing byte offsets
    1-4 when only offsets 0-3 are valid.

    This patch updates the bounds-checking to use bit offsets and ranges
    wherever possible - for concrete offsets and capacities.  In the above
    accessing "pasid" is treated as bits 8-27 of a 32-bit region, fixing the
    false positive.

    Symbolic offsets and ranges are still handled at byte granularity.

    gcc/analyzer/ChangeLog:
            PR analyzer/112792
            * bounds-checking.cc
            (out_of_bounds::oob_region_creation_event_capacity): Rename
            "capacity" to "byte_capacity".  Layout fix.
            (out_of_bounds::::add_region_creation_events): Rename
            "capacity" to "byte_capacity".
            (class concrete_out_of_bounds): Rename m_out_of_bounds_range to
            m_out_of_bounds_bits and convert from a byte_range to a bit_range.
            (concrete_out_of_bounds::get_out_of_bounds_bytes): New.
            (concrete_past_the_end::concrete_past_the_end): Rename param
            "byte_bound" to "bit_bound".  Initialize m_byte_bound.
            (concrete_past_the_end::subclass_equal_p): Update for renaming
            of m_byte_bound to m_bit_bound.
            (concrete_past_the_end::m_bit_bound): New field.
            (concrete_buffer_overflow::concrete_buffer_overflow): Convert
            param "range" from byte_range to bit_range.  Rename param
            "byte_bound" to "bit_bound".
            (concrete_buffer_overflow::emit): Update for bits vs bytes.
            (concrete_buffer_overflow::describe_final_event): Split
            into...
            (concrete_buffer_overflow::describe_final_event_as_bytes): ...this
            (concrete_buffer_overflow::describe_final_event_as_bits): ...and
            this.
            (concrete_buffer_over_read::concrete_buffer_over_read): Convert
            param "range" from byte_range to bit_range.  Rename param
            "byte_bound" to "bit_bound".
            (concrete_buffer_over_read::emit): Update for bits vs bytes.
            (concrete_buffer_over_read::describe_final_event): Split into...
            (concrete_buffer_over_read::describe_final_event_as_bytes):
            ...this
            (concrete_buffer_over_read::describe_final_event_as_bits): ...and
            this.
            (concrete_buffer_underwrite::concrete_buffer_underwrite): Convert
            param "range" from byte_range to bit_range.
            (concrete_buffer_underwrite::describe_final_event): Split into...
            (concrete_buffer_underwrite::describe_final_event_as_bytes):
            ...this
            (concrete_buffer_underwrite::describe_final_event_as_bits): ...and
            this.
            (concrete_buffer_under_read::concrete_buffer_under_read): Convert
            param "range" from byte_range to bit_range.
            (concrete_buffer_under_read::describe_final_event): Split into...
            (concrete_buffer_under_read::describe_final_event_as_bytes):
            ...this
            (concrete_buffer_under_read::describe_final_event_as_bits): ...and
            this.
            (region_model::check_region_bounds): Use bits for concrete values,
            and rename locals to indicate whether we're dealing with bits or
            bytes.  Specifically, replace "num_bytes_sval" with
            "num_bits_sval", and get it from reg's "get_bit_size_sval".
            Replace "num_bytes_tree" with "num_bits_tree".  Rename "capacity"
            to "byte_capacity".  Rename "cst_capacity_tree" to
            "cst_byte_capacity_tree".  Replace "offset" and
            "num_bytes_unsigned" with "bit_offset" and "num_bits_unsigned"
            respectively, converting from byte_offset_t to bit_offset_t.
            Replace "out" and "read_bytes" with "bits_outside" and "read_bits"
            respectively, converting from byte_range to bit_range.  Convert
            "buffer" from byte_range to bit_range.  Replace "byte_bound" with
            "bit_bound".
            * region.cc (region::get_bit_size_sval): New.
            (offset_region::get_bit_offset): New.
            (offset_region::get_bit_size_sval): New.
            (sized_region::get_bit_size_sval): New.
            (bit_range_region::get_bit_size_sval): New.
            * region.h (region::get_bit_size_sval): New vfunc.
            (offset_region::get_bit_offset): New decl.
            (offset_region::get_bit_size_sval): New decl.
            (sized_region::get_bit_size_sval): New decl.
            (bit_range_region::get_bit_size_sval): New decl.
            * store.cc (bit_range::intersects_p): New, based on
            byte_range::intersects_p.
            (bit_range::exceeds_p): New, based on byte_range::exceeds_p.
            (bit_range::falls_short_of_p): New, based on
            byte_range::falls_short_of_p.
            (byte_range::intersects_p): Delete.
            (byte_range::exceeds_p): Delete.
            (byte_range::falls_short_of_p): Delete.
            * store.h (bit_range::intersects_p): New overload.
            (bit_range::exceeds_p): New.
            (bit_range::falls_short_of_p): New.
            (byte_range::intersects_p): Delete.
            (byte_range::exceeds_p): Delete.
            (byte_range::falls_short_of_p): Delete.

    gcc/testsuite/ChangeLog:
            PR analyzer/112792
            * c-c++-common/analyzer/out-of-bounds-pr112792.c: New test.

    Signed-off-by: David Malcolm <dmalcolm@redhat.com>

[Bug analyzer/112792] -Wanalyzer-out-of-bounds false positives seen on Linux kernel with certain unions

[cvs-commit at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=112792

--- Comment #3 from GCC Commits <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by David Malcolm <dmalcolm@gcc.gnu.org>:

https://gcc.gnu.org/g:7abc7aae564e63173fbaa14805e3dddea7f6a160

commit r14-6635-g7abc7aae564e63173fbaa14805e3dddea7f6a160
Author: David Malcolm <dmalcolm@redhat.com>
Date:   Sat Dec 16 16:19:36 2023 -0500

    analyzer: add sarif properties for bounds checking diagnostics

    As a followup to r14-6057-g12b67d1e13b3cf, add SARIF property bags
    for -Wanalyzer-out-of-bounds, to help with debugging these warnings.
    This was very helpful with PR analyzer/112792.

    gcc/analyzer/ChangeLog:
            * analyzer.cc: Include "tree-pretty-print.h" and
            "diagnostic-event-id.h".
            (tree_to_json): New.
            (diagnostic_event_id_to_json): New.
            (bit_offset_to_json): New.
            (byte_offset_to_json): New.
            * analyzer.h (tree_to_json): New decl.
            (diagnostic_event_id_to_json): New decl.
            (bit_offset_to_json): New decl.
            (byte_offset_to_json): New decl.
            * bounds-checking.cc: Include "diagnostic-format-sarif.h".
            (out_of_bounds::maybe_add_sarif_properties): New.
            (concrete_out_of_bounds::maybe_add_sarif_properties): New.
            (concrete_past_the_end::maybe_add_sarif_properties): New.
            (symbolic_past_the_end::maybe_add_sarif_properties): New.
            * region-model.cc (region_to_value_map::to_json): New.
            (region_model::to_json): New.
            * region-model.h (region_to_value_map::to_json): New decl.
            (region_model::to_json): New decl.
            * store.cc (bit_range::to_json): New.
            (byte_range::to_json): New.
            * store.h (bit_range::to_json): New decl.
            (byte_range::to_json): New decl.

    Signed-off-by: David Malcolm <dmalcolm@redhat.com>

[Bug analyzer/112792] -Wanalyzer-out-of-bounds false positives seen on Linux kernel with certain unions

[dmalcolm at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=112792

--- Comment #4 from David Malcolm <dmalcolm at gcc dot gnu.org> ---
Should be fixed on trunk by r14-6622-g5f1bed2a7af828103ca23a3546466a23e8dd2f30

Keeping open to track backporting to GCC 13.

[Bug analyzer/112792] [13 Regression] -Wanalyzer-out-of-bounds false positives seen on Linux kernel with certain unions

[pinskia at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=112792

Andrew Pinski <pinskia at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|---                         |13.3

[Bug analyzer/112792] -Wanalyzer-out-of-bounds false positives seen on Linux kernel with certain unions

[dmalcolm at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=112792

David Malcolm <dmalcolm at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
         Resolution|---                         |FIXED
            Summary|[13 Regression]             |-Wanalyzer-out-of-bounds
                   |-Wanalyzer-out-of-bounds    |false positives seen on
                   |false positives seen on     |Linux kernel with certain
                   |Linux kernel with certain   |unions
                   |unions                      |
             Status|ASSIGNED                    |RESOLVED

--- Comment #5 from David Malcolm <dmalcolm at gcc dot gnu.org> ---
Unfortunately, backporting to GCC 13 is too involved (the code has greatly
changed since).

Closing this out instead.