LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Thiebaud Weksteen <tweek@google.com>
To: luc.vanoostenryck@gmail.com
Cc: jgg@ziepe.ca, Christopher Li <sparse@chrisli.org>,
	Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>,
	Nayna Jain <nayna@linux.vnet.ibm.com>,
	linux-integrity@vger.kernel.org, linux-kernel@vger.kernel.org,
	linux-sparse@vger.kernel.org, josh@joshtriplett.org
Subject: Re: [PATCH v2 1/4] tpm: Add explicit endianness cast
Date: Tue, 24 Apr 2018 09:57:58 +0000	[thread overview]
Message-ID: <CA+zpnLeJ_aKnmiQaOwRERj43uAFSaJQsWnSKJVzsJJB0VW-52w@mail.gmail.com> (raw)
In-Reply-To: <20180423100629.effueb6i7q3hmhu3@ltop.local>

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 <jgg@ziepe.ca> 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 <jgg@ziepe.ca>
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 <jgg@ziepe.ca>
> > wrote:
> > > > > >
> > > > > > > On Thu, Apr 12, 2018 at 12:13:47PM +0200, Thiebaud Weksteen
wrote:
> > > > > > > > Signed-off-by: Thiebaud Weksteen <tweek@google.com>
> > > > > > > >  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

  reply	other threads:[~2018-04-24  9:58 UTC|newest]

Thread overview: 21+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-04-12 10:13 [PATCH v2 0/4] Refactor TPM event log code Thiebaud Weksteen
2018-04-12 10:13 ` [PATCH v2 1/4] tpm: Add explicit endianness cast Thiebaud Weksteen
2018-04-17  3:02   ` Jason Gunthorpe
2018-04-17  8:32     ` Thiebaud Weksteen
2018-04-17 14:00       ` Jason Gunthorpe
2018-04-19 13:09         ` Thiebaud Weksteen
2018-04-20 14:57           ` Jason Gunthorpe
2018-04-23  9:22             ` Thiebaud Weksteen
2018-04-23 10:06               ` Luc Van Oostenryck
2018-04-24  9:57                 ` Thiebaud Weksteen [this message]
2018-04-20  5:39   ` Jarkko Sakkinen
2018-04-12 10:13 ` [PATCH v2 2/4] tpm: Move eventlog files to a subdirectory Thiebaud Weksteen
2018-04-20  5:39   ` Jarkko Sakkinen
2018-05-07 20:36     ` Mimi Zohar
2018-05-09 14:51       ` Petr Vorel
2018-05-11  1:51         ` Mimi Zohar
2018-05-10  1:39       ` Jarkko Sakkinen
2018-04-12 10:13 ` [PATCH v2 3/4] tpm: Move shared eventlog functions to common.c Thiebaud Weksteen
2018-04-20  5:40   ` Jarkko Sakkinen
2018-04-12 10:13 ` [PATCH v2 4/4] tpm: Move eventlog declarations to its own header Thiebaud Weksteen
2018-04-20  5:40   ` Jarkko Sakkinen

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=CA+zpnLeJ_aKnmiQaOwRERj43uAFSaJQsWnSKJVzsJJB0VW-52w@mail.gmail.com \
    --to=tweek@google.com \
    --cc=jarkko.sakkinen@linux.intel.com \
    --cc=jgg@ziepe.ca \
    --cc=josh@joshtriplett.org \
    --cc=linux-integrity@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-sparse@vger.kernel.org \
    --cc=luc.vanoostenryck@gmail.com \
    --cc=nayna@linux.vnet.ibm.com \
    --cc=sparse@chrisli.org \
    --subject='Re: [PATCH v2 1/4] tpm: Add explicit endianness cast' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).