linux-kernel - Re: [PATCH] x86/xen: silence smatch warning in pmu_msr_chk

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] [day] [month] [year] [list]

Message-ID: <Y1JAUj2gPSQv6esQ@kadam>
Date:   Fri, 21 Oct 2022 09:46:42 +0300
From:   Dan Carpenter <dan.carpenter@...cle.com>
To:     Boris Ostrovsky <boris.ostrovsky@...cle.com>
Cc:     Juergen Gross <jgross@...e.com>, Jan Beulich <jbeulich@...e.com>,
        Thomas Gleixner <tglx@...utronix.de>,
        Ingo Molnar <mingo@...hat.com>, Borislav Petkov <bp@...en8.de>,
        Dave Hansen <dave.hansen@...ux.intel.com>,
        "H. Peter Anvin" <hpa@...or.com>, xen-devel@...ts.xenproject.org,
        linux-kernel@...r.kernel.org, x86@...nel.org
Subject: Re: [PATCH] x86/xen: silence smatch warning in pmu_msr_chk_emulated()

On Thu, Oct 20, 2022 at 10:22:17AM -0400, Boris Ostrovsky wrote:
> 
> On 10/20/22 9:34 AM, Juergen Gross wrote:
> > On 20.10.22 15:16, Jan Beulich wrote:
> > > On 20.10.2022 13:37, Juergen Gross wrote:
> > > > Commit 8714f7bcd3c2 ("xen/pv: add fault recovery control to pmu msr
> > > > accesses") introduced code resulting in a warning issued by the smatch
> > > > static checker, claiming to use an uninitialized variable.
> > > > 
> > > > This is a false positive, but work around the warning nevertheless.
> > > 
> > > The risk of introducing a problem might be quite low here, but in general
> > > it exists: With the adjustment you remove any chance of the compiler
> > > spotting a missing initialization before use. And I'm not convinced using
> > > 0 in such a case would actually be ending up sufficiently benign.
> > 
> > Hmm, an alternative would be to initialize it to -1 and add a test for the
> > index to be >= 0 before using it.
> > 
> > Or to live with the smash warning with the chance, that a compiler might be
> > warning for the same reason in the future.
> 
> 
> Is smatch complaining about both variables or just index?

Just "index".

> There are two cases in is_intel_pmu_msr() where it returns true but
> index is not set so perhaps that's what bothers smatch?

Yep.  The "index" variable *is* undefined when it's passed so Smatch
is correct in what it's saying.  But it's is not used on that path
inside the function so it's harmless.

> It shold not complain if is_intel_pmu_msr() returns false.

Correct.

I kind of like the patch.  We generally say "fix the checker and don't
silence the warning" but in this case I feel like the checker is doing
the best possible thing and I'm not going to fix it.  Trying to silence
this warning in Smatch would come with some real downsides.

regards,
dan carpenter