From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756911AbeDXJ6P (ORCPT ); Tue, 24 Apr 2018 05:58:15 -0400 Received: from mail-io0-f194.google.com ([209.85.223.194]:44047 "EHLO mail-io0-f194.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1756841AbeDXJ6K (ORCPT ); Tue, 24 Apr 2018 05:58:10 -0400 X-Google-Smtp-Source: AIpwx48+ut8Equ6WLV2EaHwtZnSH+NOyAdE/rn08onBZ8HGCO3FgK4cK3eUOu0d1ez5YbY/YykChuXk6ihdr9fSaHGA= MIME-Version: 1.0 References: <20180412101350.210547-1-tweek@google.com> <20180412101350.210547-2-tweek@google.com> <20180417030202.GA30624@ziepe.ca> <20180417140013.GA2029@ziepe.ca> <20180420145740.GC30433@ziepe.ca> <20180423100629.effueb6i7q3hmhu3@ltop.local> In-Reply-To: <20180423100629.effueb6i7q3hmhu3@ltop.local> From: Thiebaud Weksteen Date: Tue, 24 Apr 2018 09:57:58 +0000 Message-ID: Subject: Re: [PATCH v2 1/4] tpm: Add explicit endianness cast To: luc.vanoostenryck@gmail.com Cc: jgg@ziepe.ca, Christopher Li , Jarkko Sakkinen , Nayna Jain , linux-integrity@vger.kernel.org, linux-kernel@vger.kernel.org, linux-sparse@vger.kernel.org, josh@joshtriplett.org Content-Type: text/plain; charset="UTF-8" Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Mon, Apr 23, 2018 at 12:06 PM Luc Van Oostenryck < luc.vanoostenryck@gmail.com> wrote: > On Mon, Apr 23, 2018 at 09:22:06AM +0000, Thiebaud Weksteen wrote: > > On Fri, Apr 20, 2018 at 4:57 PM Jason Gunthorpe wrote: > > > > > On Thu, Apr 19, 2018 at 01:09:12PM +0000, Thiebaud Weksteen wrote: > > > > On Tue, Apr 17, 2018 at 4:00 PM Jason Gunthorpe wrote: > > > > > > > > > On Tue, Apr 17, 2018 at 08:32:33AM +0000, Thiebaud Weksteen wrote: > > > > > > On Tue, Apr 17, 2018 at 5:02 AM Jason Gunthorpe > > wrote: > > > > > > > > > > > > > On Thu, Apr 12, 2018 at 12:13:47PM +0200, Thiebaud Weksteen wrote: > > > > > > > > Signed-off-by: Thiebaud Weksteen > > > > > > > > drivers/char/tpm/tpm_eventlog_of.c | 4 ++-- > > > > > > > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > > > > > > > > > > > > > diff --git a/drivers/char/tpm/tpm_eventlog_of.c > > > > > > b/drivers/char/tpm/tpm_eventlog_of.c > > > > > > > > index 96fd5646f866..d74568d58a66 100644 > > > > > > > > +++ b/drivers/char/tpm/tpm_eventlog_of.c > > > > > > > > @@ -56,8 +56,8 @@ int tpm_read_log_of(struct tpm_chip *chip) > > > > > > > > * but physical tpm needs the conversion. > > > > > > > > */ > > > > > > > > if (of_property_match_string(np, "compatible", > > "IBM,vtpm") < > > > > 0) { > > > > > > > > - size = be32_to_cpup(sizep); > > > > > > > > - base = be64_to_cpup(basep); > > > > > > > > + size = be32_to_cpup((__be32 *)sizep); > > > > > > > > + base = be64_to_cpup((__be64 *)basep); > > > > > > > > > > > > > Er, no.. change the definitions of sizep and basep to be __be > > > > > > > > > > > > > Jason > > > > > > > > > > > > Please read the comment before the condition. sizep and > > > > > > basep may contain either little endian or big endian and this block > > is > > > > used > > > > > > to adjust that. Let me know if there is a better way for handling > > this. > > > > > > > > > Well a cast like that will throw sparse warnings, you need __force at > > > > > least > > > > > > > > I don't think so. Since the variable is only defined as u32*, no > > specific > > > > warning is generated. I've used `make C=2 drivers/char/tpm/` with this > > > > patch applied and no new warning is being triggered. > Interesting. > > > I'm surprised to hear you say that.. > Same for me. > > > Sparse is supposed to require force on all cast that change the > > > annotation, and there are many examples in the kernel that have force > > > in that case. > Yes, sparse is supposed to warn in such cases and the __force is there > to quiets the warning when it is known that the cast is in fact correct. > > +linux-sparse@vger.kernel.org and sparse@chrisli.org for a sanity check. > > > > If you look at the man page of sparse, under the bitwise option, it states: > > "Sparse will warn on [...] any conversion of one restricted type into > > another, except via a cast that includes __attribute__((force)).". In our > > case, it is a conversion from unrestricted to restricted which does not > > fall in this category. > The man page is not very clear here. It must be understood in the context > where each use of '__bitwise' will create a new *distinct* type. > Given this and the normal type checking, sparse should warn > "on any conversion of one restricted type into another *or* between a > restricted and the corresponding plain/unrestricted type" (or consider > that an 'unrestricted type' is 'a restricted type with no restriction', > which is, I think, what was meant here). Thanks for the explanation, that make sense. I believe the issue happens when dealing with restricted pointer types more than regular types. Also, this is not new and has probably been going on for the last 13 years. For instance, 81179bb6a54c2c626b4cbcc084ca974bb2d7f2a3 explicitly removed the __force attribute. I'll send a validation patch for sparse and update this patch. > The fact that sparse doesn't warn in your case is clearly a bug in > sparse's type checking. > Regards, > -- Luc Van Oostenryck