[PATCH -next] efi/tpm: fix a compilation warning

[PATCH -next] efi/tpm: fix a compilation warning

[Qian Cai]

The linux-next "tpm: Reserve the TPM final events table" [1] introduced
a compilation warning,

drivers/firmware/efi/tpm.c: In function 'efi_tpm_eventlog_init':
drivers/firmware/efi/tpm.c:80:10: warning: passing argument 1 of
'tpm2_calc_event_log_size' makes pointer from integer without a cast
[-Wint-conversion]
  tbl_size = tpm2_calc_event_log_size(efi.tpm_final_log
drivers/firmware/efi/tpm.c:19:43: note: expected 'void *' but argument
is of type 'long unsigned int'

Fix it by making a necessary cast for the argument 1 of
tpm2_calc_event_log_size().

[1] https://lore.kernel.org/linux-efi/20190520205501.177637-3-matthewgarrett@google.com/

Signed-off-by: Qian Cai <cai@lca.pw>
---
 drivers/firmware/efi/tpm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/firmware/efi/tpm.c b/drivers/firmware/efi/tpm.c
index 74d0cd1647b8..1d3f5ca3eaaf 100644
--- a/drivers/firmware/efi/tpm.c
+++ b/drivers/firmware/efi/tpm.c
@@ -75,7 +75,7 @@ int __init efi_tpm_eventlog_init(void)
 		goto out;
 	}
 
-	tbl_size = tpm2_calc_event_log_size(efi.tpm_final_log
+	tbl_size = tpm2_calc_event_log_size((void *)efi.tpm_final_log
 					    + sizeof(final_tbl->version)
 					    + sizeof(final_tbl->nr_events),
 					    final_tbl->nr_events,
-- 
2.20.1 (Apple Git-117)

Re: [PATCH -next] efi/tpm: fix a compilation warning

[Arnd Bergmann]

On Tue, Jun 11, 2019 at 3:59 PM Qian Cai <cai@lca.pw> wrote:
>
> The linux-next "tpm: Reserve the TPM final events table" [1] introduced
> a compilation warning,
>
> drivers/firmware/efi/tpm.c: In function 'efi_tpm_eventlog_init':
> drivers/firmware/efi/tpm.c:80:10: warning: passing argument 1 of
> 'tpm2_calc_event_log_size' makes pointer from integer without a cast
> [-Wint-conversion]
>   tbl_size = tpm2_calc_event_log_size(efi.tpm_final_log
> drivers/firmware/efi/tpm.c:19:43: note: expected 'void *' but argument
> is of type 'long unsigned int'
>
> Fix it by making a necessary cast for the argument 1 of
> tpm2_calc_event_log_size().
>
> [1] https://lore.kernel.org/linux-efi/20190520205501.177637-3-matthewgarrett@google.com/
>
> Signed-off-by: Qian Cai <cai@lca.pw>

I see the same build warning, but I don't think adding a cast here
solves the problem:

- efi.tpm_final_log is a physical address that gets passed into
  memremap() to return a pointer
- tpm2_calc_event_log_size() takes a pointer argument and
  dereferences it.

My best guess is that we should pass the output of memremap()
here rather than the input:

--- a/drivers/firmware/efi/tpm.c
+++ b/drivers/firmware/efi/tpm.c
@@ -75,7 +75,7 @@ int __init efi_tpm_eventlog_init(void)
                goto out;
        }

-       tbl_size = tpm2_calc_event_log_size(efi.tpm_final_log
+       tbl_size = tpm2_calc_event_log_size(final_tbl
                                            + sizeof(final_tbl->version)
                                            + sizeof(final_tbl->nr_events),
                                            final_tbl->nr_events,

No idea if that is actually what was intended here, but it makes
the code look more plausible.

        Arnd

Re: [PATCH -next] efi/tpm: fix a compilation warning

[Bartosz Szczepanek]

On Thu, Jun 13, 2019 at 10:55 AM Arnd Bergmann <arnd@arndb.de> wrote:
>
> - efi.tpm_final_log is a physical address that gets passed into
>   memremap() to return a pointer
> - tpm2_calc_event_log_size() takes a pointer argument and
>   dereferences it.

Where does it? It's passed with some added offset to
__calc_tpm2_event_size, which does the remapping part. That's why
physical address is used here.

> My best guess is that we should pass the output of memremap()
> here rather than the input:
>
> --- a/drivers/firmware/efi/tpm.c
> +++ b/drivers/firmware/efi/tpm.c
> @@ -75,7 +75,7 @@ int __init efi_tpm_eventlog_init(void)
>                 goto out;
>         }
>
> -       tbl_size = tpm2_calc_event_log_size(efi.tpm_final_log
> +       tbl_size = tpm2_calc_event_log_size(final_tbl
>                                             + sizeof(final_tbl->version)
>                                             + sizeof(final_tbl->nr_events),
>                                             final_tbl->nr_events,
>
> No idea if that is actually what was intended here, but it makes
> the code look more plausible.

Passing final_tbl will lead to failure, as it will be remapped for
second time. Cast is needed here. Changing the type from void* to
unsigned long or phys_addr_t (and casting later) would also do the
job. I'm fine with both options.

Re: [PATCH -next] efi/tpm: fix a compilation warning

[Arnd Bergmann]

On Thu, Jun 13, 2019 at 1:41 PM Bartosz Szczepanek <bsz@semihalf.com> wrote:
>
> On Thu, Jun 13, 2019 at 10:55 AM Arnd Bergmann <arnd@arndb.de> wrote:
> >
> > - efi.tpm_final_log is a physical address that gets passed into
> >   memremap() to return a pointer
> > - tpm2_calc_event_log_size() takes a pointer argument and
> >   dereferences it.
>
> Where does it? It's passed with some added offset to
> __calc_tpm2_event_size, which does the remapping part. That's why
> physical address is used here.

Ah, right. I was confused by how __calc_tpm2_event_size()
may or may not do the mapping again based on the 'bool do_mapping'
argument, which is 'true' here.

Would it be correct to change that to 'false' then (or completely remove
the additional remap, given that the other two callers pass false
already) and pass final_tbl?

         Arnd

Re: [PATCH -next] efi/tpm: fix a compilation warning

[Bartosz Szczepanek]

On Thu, Jun 13, 2019 at 2:40 PM Arnd Bergmann <arnd@arndb.de> wrote:
>
> Would it be correct to change that to 'false' then (or completely remove
> the additional remap, given that the other two callers pass false
> already) and pass final_tbl?

The problem is that we don't know the final_tbl size before running
tpm2_calc_event_log_size on it, so we cannot map it's entire content.
Only table header is mapped at the beginning.

After size of entire table is calculated it can be mapped as a whole
and no additional remap is needed, hence the calls with do_mapping =
false.