linux-kernel - Re: [PATCH] drm/i915: Remove tautological compare in eb_relocate

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 03 Dec 2019 13:42:47 +0000
From:   Chris Wilson <chris@...is-wilson.co.uk>
To:     Nick Desaulniers <ndesaulniers@...gle.com>
Cc:     Jani Nikula <jani.nikula@...ux.intel.com>,
        Joonas Lahtinen <joonas.lahtinen@...ux.intel.com>,
        Nathan Chancellor <natechancellor@...il.com>,
        Rodrigo Vivi <rodrigo.vivi@...el.com>,
        intel-gfx@...ts.freedesktop.org,
        dri-devel <dri-devel@...ts.freedesktop.org>,
        LKML <linux-kernel@...r.kernel.org>,
        clang-built-linux <clang-built-linux@...glegroups.com>
Subject: Re: [PATCH] drm/i915: Remove tautological compare in eb_relocate_vma

Quoting Nick Desaulniers (2019-12-02 19:18:20)
> On Sat, Nov 23, 2019 at 12:05 PM Chris Wilson <chris@...is-wilson.co.uk> wrote:
> >
> > Quoting Nathan Chancellor (2019-11-23 19:53:22)
> > > -Wtautological-compare was recently added to -Wall in LLVM, which
> > > exposed an if statement in i915 that is always false:
> > >
> > > ../drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c:1485:22: warning:
> > > result of comparison of constant 576460752303423487 with expression of
> > > type 'unsigned int' is always false
> > > [-Wtautological-constant-out-of-range-compare]
> > >         if (unlikely(remain > N_RELOC(ULONG_MAX)))
> > >             ~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~
> > >
> > > Since remain is an unsigned int, it can never be larger than UINT_MAX,
> > > which is less than ULONG_MAX / sizeof(struct drm_i915_gem_relocation_entry).
> > > Remove this statement to fix the warning.
> >
> > The check should remain as we do want to document the overflow
> > calculation, and it should represent the types used -- it's much easier
> 
> What do you mean "represent the types used?"  Are you concerned that
> the type of drm_i915_gem_exec_object2->relocation_count might change
> in the future?

We may want to change the restriction, yes.
 
> > to review a stub than trying to find a missing overflow check. If the
> > overflow cannot happen as the types are wide enough, no problem, the
> > compiler can remove the known false branch.
> 
> What overflow are you trying to protect against here?

These values are under user control, our validation steps should be
clear and easy to check. If we have the types wrong, if the checks are
wrong, we need to fix them. If the code is removed because it can be
evaluated by the compiler to be redundant, it is much harder for us to
verify that we have tried to validate user input.

> > Tautology here has a purpose for conveying information to the reader.
> 
> Well leaving a warning unaddressed is also not a solution.  Either
> replace it with a comment or turn off the warning for your subdir.

My personal preference would be to use a bunch of central macros for the
various type/kmalloc overflows, and have the warnings suppressed there
since they are very much about documenting user input validation.
-Chris