Re: [PATCH 1/2] osdep: Introduce major.h and use it

2021-07-14 Thread Daniel Kiper
On Wed, Jul 14, 2021 at 08:54:29AM +0200, Petr Vorel wrote:
> Hi Daniel,
>
> > > On Thu, Jul 08, 2021 at 05:55:57PM +0200, Petr Vorel wrote:
> > > > Signed-off-by: Petr Vorel 
> > > > ---
> > > >  grub-core/osdep/devmapper/getroot.c  |  7 +--
> > > >  grub-core/osdep/devmapper/hostdisk.c |  7 +--
> > > >  grub-core/osdep/linux/getroot.c  |  7 +--
> > > >  grub-core/osdep/unix/getroot.c   |  7 +--
> > > >  include/grub/osdep/major.h   | 30 
> > > >  5 files changed, 34 insertions(+), 24 deletions(-)
> > > >  create mode 100644 include/grub/osdep/major.h
>
> > > May I ask you to explain in the commit message why this patch is needed?
> > It's just a small cleanup. If you're not against it, sure, I can mention it 
> > in
> > the commit message.

I am OK with the cleanup.

> Ah, you probably mean to put the description from major.h also to the commit
> message.

To some extent. I think it should be phrased a bit better in the commit message.

> Kind regards,
> Petr
>
> +/*
> + * Fix for glibc 2.25 is deprecating the namespace pollution of sys/types.h
> + * injecting major(), minor(), and makedev() into the compilation 
> environment.
> + * See configure.ac.

It seems to me "See configure.ac." is not relevant in the GRUB source.
So, probably it should be replaced with some text from glibc configure.ac.

Daniel

___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel


Re: [PATCH 2/2] grub2: use stat instead of udevadm for partition lookup

2021-07-14 Thread Daniel Kiper
On Tue, Jul 13, 2021 at 09:10:39PM +0200, Petr Vorel wrote:
> Hi all,
>
> > On 7/13/21 1:48 PM, Daniel Kiper wrote:
> > > On Thu, Jul 08, 2021 at 05:55:58PM +0200, Petr Vorel wrote:
> > >> From: Jeff Mahoney 
>
> > >> sysfs_partition_path calls udevadm to resolve the sysfs path for
> > >> a block device. That can be accomplished by stating the device node
> > >> and using the major/minor to follow the symlinks in /sys/dev/block/.
>
> > >> This cuts the execution time of grub2-mkconfig from 10s to 2s on
> > >> my system.
>
> > >> Signed-off-by: Jeff Mahoney 
> > >> [ pvorel: include grub/osdep/major.h ]
> > >> Signed-off-by: Petr Vorel 
> > >> ---
> > >>  grub-core/osdep/linux/hostdisk.c | 8 
> > >>  1 file changed, 8 insertions(+)
>
> > >> diff --git a/grub-core/osdep/linux/hostdisk.c 
> > >> b/grub-core/osdep/linux/hostdisk.c
> > >> index da62f924e..43dc4b0ba 100644
> > >> --- a/grub-core/osdep/linux/hostdisk.c
> > >> +++ b/grub-core/osdep/linux/hostdisk.c
> > >> @@ -31,6 +31,7 @@
> > >>  #include 
> > >>  #include 
> > >>  #include 
> > >> +#include 
>
> > >>  #include 
> > >>  #include 
> > >> @@ -105,6 +106,13 @@ sysfs_partition_path (const char *dev, const char 
> > >> *entry)
> > >>char *buf = NULL;
> > >>size_t len = 0;
> > >>char *path = NULL;
> > >> +  struct stat st;
> > >> +  int ret;
> > >> +
> > >> +  ret = stat(dev, &st);
>
> > > Missing space between "stat" and "(".
>
> > >> +  if (ret == 0 && S_ISBLK(st.st_mode))
>
> > > Same for S_ISBLK...
>
> > >> +return xasprintf ("/sys/dev/block/%u:%u/%s",
> > >> +  major (st.st_rdev), minor (st.st_rdev), entry);
>
> > >>argv[0] = "udevadm";
> > >>argv[1] = "info";
>
> > > Do we really need udevadm fallback mechanism? If something went wrong
> > > here for us I do not expect it will work for udevadm either.
>
> > I suspect not.  'udevadm info --query path --name ' does
> > essentially the same thing but with many more steps and the cost
> > starting an external program for every block device.  I left it in with
> > the assumption that it was probably there for a reason and perhaps
> > udevadm on early udev systems did something more special than just doing
> > stat + sysfs path building.  I didn't spend any time on validating that
> > assumption.
>
> As it'd be wort to remove udevadm fallback, I'll stop being lazy and setup VM 
> to
> test.

Please do. I prefer to remove this fallback because it will be mostly
dead code after your fix.

Daniel

___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel


Re: UEFI Secureboot not succeeding with Grub 2.06 and later version

2021-07-14 Thread Daniel Kiper
On Mon, Jul 12, 2021 at 04:20:56PM +, Sayanta Pattanayak wrote:
> Hi Daniel,
>
> Secureboot worked fine with the change(GRUB_FILE_TYPE_LINUX_KERNEL -> 
> GRUB_FILE_TYPE_NONE) you suggested.

Great!

> disk/efi/efidisk.c:531: opening hd0 succeeded
> partmap/gpt.c:93: Read a valid GPT header
> partmap/gpt.c:115: GPT entry 0: start=2048, length=40959
> partmap/gpt.c:115: GPT entry 1: start=43008, length=409599
> kern/fs.c:56: Detecting ext2...
> kern/verifiers.c:88: file: /Image type: 0
> disk/efi/efidisk.c:593: reading 0x40 sectors at the sector 0xcc40 from hd0
> loader/arm64/linux.c:61: UEFI stub kernel:
> loader/arm64/linux.c:62: PE/COFF header @ 0040
> loader/arm64/linux.c:316: kernel file size: 34054136
> loader/arm64/linux.c:318: kernel numpages: 8314
> disk/efi/efidisk.c:593: reading 0x40 sectors at the sector 0xcc80 from hd0
> disk/efi/efidisk.c:593: reading 0x40 sectors at the sector 0xccc0 from hd0
> 
> loader/efi/fdt.c:63: allocating 1155 bytes for fdt
> loader/arm64/linux.c:89: Initrd @ 0xf6e2-0xf6fffa00
> loader/efi/fdt.c:97: Installed/updated FDT configuration table @ 0xf71d
> Loading driver at 0x000F4D1 EntryPoint=0x000F650EDD8
> Loading driver at 0x000F4D1 EntryPoint=0x000F650EDD8
> loader/arm64/linux.c:144: linux command line: 'BOOT_IMAGE=/Image acpi=force
> console=ttyAMA0,115200 ip=dhcp
> root=PARTUUID=9c53a91b-e182-4ff1-aeac-6ee2c432ae94 rootwait verbose debug'
> loader/arm64/linux.c:159: starting image 0xfca9a798
> EFI stub: Booting Linux Kernel...
> EFI stub: EFI_RNG_PROTOCOL unavailable
> EFI stub: UEFI Secure Boot is enabled.
> EFI stub: Using DTB from configuration table
> EFI stub: Exiting boot services and installing virtual address map...
>
> We understand LoadImage() interface is used in our platform, but if
> the error scenario is not expected with LoadImage() interface then we
> need further investigation. We are trying to look into it.
>
> What can we infer from the change you suggested and that it worked? Do
> we need to make certain changes in our platform?

The change which I suggested was just a check for my theory. It is not
real fix. We have to fix this issue in the GRUB in a different way. This
is not your fault. When we have a fix we will ask you for some tests.

Daniel

___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel


Re: [PATCH 1/2] osdep: Introduce major.h and use it

2021-07-14 Thread Petr Vorel
Hi Daniel,

> On Wed, Jul 14, 2021 at 08:54:29AM +0200, Petr Vorel wrote:
> > Hi Daniel,

> > > > On Thu, Jul 08, 2021 at 05:55:57PM +0200, Petr Vorel wrote:
> > > > > Signed-off-by: Petr Vorel 
> > > > > ---
> > > > >  grub-core/osdep/devmapper/getroot.c  |  7 +--
> > > > >  grub-core/osdep/devmapper/hostdisk.c |  7 +--
> > > > >  grub-core/osdep/linux/getroot.c  |  7 +--
> > > > >  grub-core/osdep/unix/getroot.c   |  7 +--
> > > > >  include/grub/osdep/major.h   | 30 
> > > > > 
> > > > >  5 files changed, 34 insertions(+), 24 deletions(-)
> > > > >  create mode 100644 include/grub/osdep/major.h

> > > > May I ask you to explain in the commit message why this patch is needed?
> > > It's just a small cleanup. If you're not against it, sure, I can mention 
> > > it in
> > > the commit message.

> I am OK with the cleanup.
thx for ack!

> > Ah, you probably mean to put the description from major.h also to the commit
> > message.

> To some extent. I think it should be phrased a bit better in the commit 
> message.
sure.

> > Kind regards,
> > Petr

> > +/*
> > + * Fix for glibc 2.25 is deprecating the namespace pollution of sys/types.h
> > + * injecting major(), minor(), and makedev() into the compilation 
> > environment.
> > + * See configure.ac.

> It seems to me "See configure.ac." is not relevant in the GRUB source.
> So, probably it should be replaced with some text from glibc configure.ac.

I didn't know how to link AC_HEADER_MAJOR with include/grub/osdep/major.h.
Because in the future when glibc 2.25 is old enough the header will be removed
and AC_HEADER_MAJOR might be left in configure.ac (name is different from
MAJOR_IN_{MKDEV,SYSMACROS}). But I can note this in commit message instead.

Kind regards,
Petr

> Daniel

___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel


Re: [PATCH 1/2] osdep: Introduce major.h and use it

2021-07-14 Thread Petr Vorel
Hi Daniel,

...
> > > +/*
> > > + * Fix for glibc 2.25 is deprecating the namespace pollution of 
> > > sys/types.h
> > > + * injecting major(), minor(), and makedev() into the compilation 
> > > environment.
> > > + * See configure.ac.

> > It seems to me "See configure.ac." is not relevant in the GRUB source.
> > So, probably it should be replaced with some text from glibc configure.ac.

> I didn't know how to link AC_HEADER_MAJOR with include/grub/osdep/major.h.
> Because in the future when glibc 2.25 is old enough the header will be removed
> and AC_HEADER_MAJOR might be left in configure.ac (name is different from
> MAJOR_IN_{MKDEV,SYSMACROS}). But I can note this in commit message instead.
But thinking about it twice also note in configure.ac instead (+ commit message)
would be good. But that's just a minor detail.

Kind regards,
Petr

> Kind regards,
> Petr

> > Daniel

___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel


Re: [PATCH] ibmvtpm: Add support for trusted boot using a vTPM 2.0

2021-07-14 Thread Daniel Kiper
CC-ing folks CC-ed in Daniel's patch series and Eric.

On Mon, Jul 12, 2021 at 03:02:19PM -0400, Stefan Berger wrote:
> From: Stefan Berger 
>
> Add support for trusted boot using a vTPM 2.0 on the IBM ieee1275
> platform. With this patch grub now measures text and binary data
> into the TPM's PCRs 8 and 9 in the same way as the x86_64 platform
> does.
>
> This patch requires Daniel Axtens's patches for claiming more memory.

Would not be it simpler if you take out the memory mgmt changes patches
from Daniel's patch series?

Daniel, are you OK with it?

Additionally, please CC Eric when you post IEEE1275 patches next time.

> Signed-off-by: Stefan Berger 
> ---
>  grub-core/Makefile.core.def   |   8 ++
>  grub-core/commands/ieee1275/ibmvtpm.c | 118 ++
>  grub-core/kern/ieee1275/ibmvtpm.c |  62 ++
>  include/grub/ieee1275/ibmvtpm.h   |  32 +++
>  4 files changed, 220 insertions(+)
>  create mode 100644 grub-core/commands/ieee1275/ibmvtpm.c
>  create mode 100644 grub-core/kern/ieee1275/ibmvtpm.c
>  create mode 100644 include/grub/ieee1275/ibmvtpm.h
>
> diff --git a/grub-core/Makefile.core.def b/grub-core/Makefile.core.def
> index 3f3459b2c..e2a64f8ff 100644
> --- a/grub-core/Makefile.core.def
> +++ b/grub-core/Makefile.core.def
> @@ -1166,6 +1166,14 @@ module = {
>enable = powerpc_ieee1275;
>  };
>
> +module = {
> +  name = tpm;
> +  common = commands/tpm.c;
> +  common = kern/ieee1275/ibmvtpm.c;
> +  ieee1275 = commands/ieee1275/ibmvtpm.c;

s/ieee1275/common/?

> +  enable = powerpc_ieee1275;
> +};
> +
>  module = {
>name = terminal;
>common = commands/terminal.c;
> diff --git a/grub-core/commands/ieee1275/ibmvtpm.c 
> b/grub-core/commands/ieee1275/ibmvtpm.c
> new file mode 100644
> index 0..9b06c76d9
> --- /dev/null
> +++ b/grub-core/commands/ieee1275/ibmvtpm.c
> @@ -0,0 +1,118 @@
> +/*
> + *  GRUB  --  GRand Unified Bootloader
> + *  Copyright (C) 2021  IBM Corporation

Copyright (C) 2021  Free Software Foundation, Inc.

Or both if you strongly need IBM...

> + *  GRUB is free software: you can redistribute it and/or modify
> + *  it under the terms of the GNU General Public License as published by
> + *  the Free Software Foundation, either version 3 of the License, or
> + *  (at your option) any later version.
> + *
> + *  GRUB is distributed in the hope that it will be useful,
> + *  but WITHOUT ANY WARRANTY; without even the implied warranty of
> + *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + *  GNU General Public License for more details.
> + *
> + *  You should have received a copy of the GNU General Public License
> + *  along with GRUB.  If not, see .
> + *
> + *  IBM vTPM support code.
> + */
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +static grub_ieee1275_ihandle_t grub_tpm_ihandle;
> +static grub_uint8_t grub_tpm_version;
> +
> +static void grub_ieee1275_tpm_init (grub_ieee1275_ihandle_t *tpm_ihandle)

You can drop from static functions, variables, etc. names "grub_" prefix.

> +{
> +  static int init_called = 0;
> +
> +  if (!init_called) {
> +  init_called = 1;
> +  grub_ieee1275_open ("/vdevice/vtpm", &grub_tpm_ihandle);
> +  }

Incorrect formatting. It should be

if (!init_called)
  {
init_called = 1;
grub_ieee1275_open ("/vdevice/vtpm", &grub_tpm_ihandle);
  }

You can always refer to grub-core/kern/efi/sb.c to get formatting
examples for various pieces of the GRUB code.

Please fix similar issues below too.

> +  *tpm_ihandle = grub_tpm_ihandle;
> +}
> +
> +static grub_err_t
> +grub_tpm_get_tpm_version (grub_uint8_t *protocol_version)
> +{
> +  static int version_probed = 0;
> +  grub_ieee1275_phandle_t vtpm;
> +  char buffer[20];
> +  grub_ssize_t buffer_size;
> +
> +  if (!version_probed) {
> + version_probed = 1;
> + if (!grub_ieee1275_finddevice ("/vdevice/vtpm", &vtpm) &&
> + !grub_ieee1275_get_property (vtpm, "compatible", buffer,
> +  sizeof buffer, &buffer_size) &&
> + !grub_strcmp (buffer, "IBM,vtpm20")) {
> +   grub_tpm_version = 2;
> +}
> +  }
> +  *protocol_version = grub_tpm_version;
> +
> +  return 0;

You ignore this value later.

I think the prototype of this function should be following:
  grub_uint8_t get_tpm_version (void)

Which means you should return the TPM version.

> +}
> +
> +static grub_int8_t

static grub_err_t

> +grub_tpm_handle_find (grub_ieee1275_ihandle_t *tpm_handle,
> +   grub_uint8_t *protocol_version)
> +{
> +  grub_ieee1275_tpm_init (tpm_handle);
> +  if (*tpm_handle == NULL)
> +  return 0;

return GRUB_ERR_UNKNOWN_DEVICE;

> +  grub_tpm_get_tpm_version (protocol_version);
> +
> +  return 1;

return GRUB_ERR_NONE;

> +}
> +
> +static grub_err_t
> +grub_tpm2_log_event (grub_ieee1275_ihandle_t tpm_handle, unsigned char *buf,
> +  grub_size_t size, gru

Re: [PATCH v2 01/22] ieee1275: drop HEAP_MAX_ADDR, HEAP_MIN_SIZE

2021-07-14 Thread Daniel Kiper
On Wed, Jun 30, 2021 at 06:40:10PM +1000, Daniel Axtens wrote:
> HEAP_MAX_ADDR is confusing. Currently it is set to 32MB, except
> on ieee1275 on x86, where it is 64MB.
>
> There is a comment which purports to explain it:
>
> /* If possible, we will avoid claiming heap above this address, because it
>seems to cause relocation problems with OSes that link at 4 MiB */
>
> This doesn't make a lot of sense when the constants are well above 4MB
> already. It was not always this way. Prior to
> commit 7b5d0fe4440c ("Increase heap limit") in 2010, HEAP_MAX_SIZE and
> HEAP_MAX_ADDR were indeed 4MB. However, when the constants were increased
> the comment was left unchanged.
>
> It's been over a decade. It doesn't seem like we have problems with
> claims over 4MB on powerpc or x86 ieee1275. (sparc does things completely
> differently and never used the constant.)
>
> Drop the constant and the check.
>
> The only use of HEAP_MIN_SIZE was to potentially override the
> HEAP_MAX_ADDR check. It is now unused. Remove it.
>
> Signed-off-by: Daniel Axtens 

Reviewed-by: Daniel Kiper 

> ---
>  grub-core/kern/ieee1275/init.c | 17 -
>  1 file changed, 17 deletions(-)
>
> diff --git a/grub-core/kern/ieee1275/init.c b/grub-core/kern/ieee1275/init.c
> index d483e35eed2b..c5d091689f29 100644
> --- a/grub-core/kern/ieee1275/init.c
> +++ b/grub-core/kern/ieee1275/init.c
> @@ -45,9 +45,6 @@
>  #include 
>  #endif
>
> -/* The minimal heap size we can live with. */
> -#define HEAP_MIN_SIZE(unsigned long) (2 * 1024 * 1024)
> -
>  /* The maximum heap size we're going to claim */
>  #ifdef __i386__
>  #define HEAP_MAX_SIZE(unsigned long) (64 * 1024 * 1024)
> @@ -55,14 +52,6 @@
>  #define HEAP_MAX_SIZE(unsigned long) (32 * 1024 * 1024)
>  #endif
>
> -/* If possible, we will avoid claiming heap above this address, because it
> -   seems to cause relocation problems with OSes that link at 4 MiB */
> -#ifdef __i386__
> -#define HEAP_MAX_ADDR(unsigned long) (64 * 1024 * 1024)
> -#else
> -#define HEAP_MAX_ADDR(unsigned long) (32 * 1024 * 1024)
> -#endif
> -
>  extern char _start[];
>  extern char _end[];
>
> @@ -183,12 +172,6 @@ heap_init (grub_uint64_t addr, grub_uint64_t len, 
> grub_memory_type_t type,
>if (*total + len > HEAP_MAX_SIZE)
>  len = HEAP_MAX_SIZE - *total;
>
> -  /* Avoid claiming anything above HEAP_MAX_ADDR, if possible. */
> -  if ((addr < HEAP_MAX_ADDR) &&  /* if it's too 
> late, don't bother */
> -  (addr + len > HEAP_MAX_ADDR) &&/* if 
> it wasn't available anyway, don't bother */
> -  (*total + (HEAP_MAX_ADDR - addr) > HEAP_MIN_SIZE)) /* only limit 
> ourselves when we can afford to */
> - len = HEAP_MAX_ADDR - addr;
> -
>/* In theory, firmware should already prevent this from happening by not
>   listing our own image in /memory/available.  The check below is intended
>   as a safeguard in case that doesn't happen.  However, it doesn't protect
> --
> 2.30.2

Daniel

___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel


Re: [PATCH] ibmvtpm: Add support for trusted boot using a vTPM 2.0

2021-07-14 Thread Stefan Berger


On 7/14/21 12:16 PM, Daniel Kiper wrote:

CC-ing folks CC-ed in Daniel's patch series and Eric.

On Mon, Jul 12, 2021 at 03:02:19PM -0400, Stefan Berger wrote:

From: Stefan Berger 

Add support for trusted boot using a vTPM 2.0 on the IBM ieee1275
platform. With this patch grub now measures text and binary data
into the TPM's PCRs 8 and 9 in the same way as the x86_64 platform
does.

This patch requires Daniel Axtens's patches for claiming more memory.

Would not be it simpler if you take out the memory mgmt changes patches
from Daniel's patch series?
You mean reposting this series with his 3 patches upfront? I can 
certainly do that.


Daniel, are you OK with it?

Additionally, please CC Eric when you post IEEE1275 patches next time.



Will do.





Signed-off-by: Stefan Berger 
---
  grub-core/Makefile.core.def   |   8 ++
  grub-core/commands/ieee1275/ibmvtpm.c | 118 ++
  grub-core/kern/ieee1275/ibmvtpm.c |  62 ++
  include/grub/ieee1275/ibmvtpm.h   |  32 +++
  4 files changed, 220 insertions(+)
  create mode 100644 grub-core/commands/ieee1275/ibmvtpm.c
  create mode 100644 grub-core/kern/ieee1275/ibmvtpm.c
  create mode 100644 include/grub/ieee1275/ibmvtpm.h

diff --git a/grub-core/Makefile.core.def b/grub-core/Makefile.core.def
index 3f3459b2c..e2a64f8ff 100644
--- a/grub-core/Makefile.core.def
+++ b/grub-core/Makefile.core.def
@@ -1166,6 +1166,14 @@ module = {
enable = powerpc_ieee1275;
  };

+module = {
+  name = tpm;
+  common = commands/tpm.c;
+  common = kern/ieee1275/ibmvtpm.c;
+  ieee1275 = commands/ieee1275/ibmvtpm.c;

s/ieee1275/common/?


+  enable = powerpc_ieee1275;
+};
+
  module = {
name = terminal;
common = commands/terminal.c;
diff --git a/grub-core/commands/ieee1275/ibmvtpm.c 
b/grub-core/commands/ieee1275/ibmvtpm.c
new file mode 100644
index 0..9b06c76d9
--- /dev/null
+++ b/grub-core/commands/ieee1275/ibmvtpm.c
@@ -0,0 +1,118 @@
+/*
+ *  GRUB  --  GRand Unified Bootloader
+ *  Copyright (C) 2021  IBM Corporation

Copyright (C) 2021  Free Software Foundation, Inc.

Or both if you strongly need IBM...


+ *  GRUB is free software: you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License as published by
+ *  the Free Software Foundation, either version 3 of the License, or
+ *  (at your option) any later version.
+ *
+ *  GRUB is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *  GNU General Public License for more details.
+ *
+ *  You should have received a copy of the GNU General Public License
+ *  along with GRUB.  If not, see .
+ *
+ *  IBM vTPM support code.
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+static grub_ieee1275_ihandle_t grub_tpm_ihandle;
+static grub_uint8_t grub_tpm_version;
+
+static void grub_ieee1275_tpm_init (grub_ieee1275_ihandle_t *tpm_ihandle)

You can drop from static functions, variables, etc. names "grub_" prefix.


+{
+  static int init_called = 0;
+
+  if (!init_called) {
+  init_called = 1;
+  grub_ieee1275_open ("/vdevice/vtpm", &grub_tpm_ihandle);
+  }

Incorrect formatting. It should be

if (!init_called)
   {
 init_called = 1;
 grub_ieee1275_open ("/vdevice/vtpm", &grub_tpm_ihandle);
   }

You can always refer to grub-core/kern/efi/sb.c to get formatting
examples for various pieces of the GRUB code.

Please fix similar issues below too.


+  *tpm_ihandle = grub_tpm_ihandle;
+}
+
+static grub_err_t
+grub_tpm_get_tpm_version (grub_uint8_t *protocol_version)
+{
+  static int version_probed = 0;
+  grub_ieee1275_phandle_t vtpm;
+  char buffer[20];
+  grub_ssize_t buffer_size;
+
+  if (!version_probed) {
+ version_probed = 1;
+ if (!grub_ieee1275_finddevice ("/vdevice/vtpm", &vtpm) &&
+ !grub_ieee1275_get_property (vtpm, "compatible", buffer,
+  sizeof buffer, &buffer_size) &&
+ !grub_strcmp (buffer, "IBM,vtpm20")) {
+   grub_tpm_version = 2;
+}
+  }
+  *protocol_version = grub_tpm_version;
+
+  return 0;

You ignore this value later.

I think the prototype of this function should be following:
   grub_uint8_t get_tpm_version (void)

Which means you should return the TPM version.


+}
+
+static grub_int8_t

static grub_err_t


+grub_tpm_handle_find (grub_ieee1275_ihandle_t *tpm_handle,
+ grub_uint8_t *protocol_version)
+{
+  grub_ieee1275_tpm_init (tpm_handle);
+  if (*tpm_handle == NULL)
+  return 0;

return GRUB_ERR_UNKNOWN_DEVICE;


+  grub_tpm_get_tpm_version (protocol_version);
+
+  return 1;

return GRUB_ERR_NONE;


+}
+
+static grub_err_t
+grub_tpm2_log_event (grub_ieee1275_ihandle_t tpm_handle, unsigned char *buf,
+grub_size_t size, grub_uint8_t pcr,
+const char *description)
+{
+

Re: [PATCH] ibmvtpm: Add support for trusted boot using a vTPM 2.0

2021-07-14 Thread Daniel Axtens
Stefan Berger  writes:

> On 7/14/21 12:16 PM, Daniel Kiper wrote:
>> CC-ing folks CC-ed in Daniel's patch series and Eric.
>>
>> On Mon, Jul 12, 2021 at 03:02:19PM -0400, Stefan Berger wrote:
>>> From: Stefan Berger 
>>>
>>> Add support for trusted boot using a vTPM 2.0 on the IBM ieee1275
>>> platform. With this patch grub now measures text and binary data
>>> into the TPM's PCRs 8 and 9 in the same way as the x86_64 platform
>>> does.
>>>
>>> This patch requires Daniel Axtens's patches for claiming more memory.
>> Would not be it simpler if you take out the memory mgmt changes patches
>> from Daniel's patch series?
> You mean reposting this series with his 3 patches upfront? I can 
> certainly do that.
>>
>> Daniel, are you OK with it?
>>

I don't mind what series the patches go through, as long as they get
merged at some point :)

Kind regards,
Daniel

>> Additionally, please CC Eric when you post IEEE1275 patches next time.
>
>
> Will do.
>
>
>>
>>> Signed-off-by: Stefan Berger 
>>> ---
>>>   grub-core/Makefile.core.def   |   8 ++
>>>   grub-core/commands/ieee1275/ibmvtpm.c | 118 ++
>>>   grub-core/kern/ieee1275/ibmvtpm.c |  62 ++
>>>   include/grub/ieee1275/ibmvtpm.h   |  32 +++
>>>   4 files changed, 220 insertions(+)
>>>   create mode 100644 grub-core/commands/ieee1275/ibmvtpm.c
>>>   create mode 100644 grub-core/kern/ieee1275/ibmvtpm.c
>>>   create mode 100644 include/grub/ieee1275/ibmvtpm.h
>>>
>>> diff --git a/grub-core/Makefile.core.def b/grub-core/Makefile.core.def
>>> index 3f3459b2c..e2a64f8ff 100644
>>> --- a/grub-core/Makefile.core.def
>>> +++ b/grub-core/Makefile.core.def
>>> @@ -1166,6 +1166,14 @@ module = {
>>> enable = powerpc_ieee1275;
>>>   };
>>>
>>> +module = {
>>> +  name = tpm;
>>> +  common = commands/tpm.c;
>>> +  common = kern/ieee1275/ibmvtpm.c;
>>> +  ieee1275 = commands/ieee1275/ibmvtpm.c;
>> s/ieee1275/common/?
>>
>>> +  enable = powerpc_ieee1275;
>>> +};
>>> +
>>>   module = {
>>> name = terminal;
>>> common = commands/terminal.c;
>>> diff --git a/grub-core/commands/ieee1275/ibmvtpm.c 
>>> b/grub-core/commands/ieee1275/ibmvtpm.c
>>> new file mode 100644
>>> index 0..9b06c76d9
>>> --- /dev/null
>>> +++ b/grub-core/commands/ieee1275/ibmvtpm.c
>>> @@ -0,0 +1,118 @@
>>> +/*
>>> + *  GRUB  --  GRand Unified Bootloader
>>> + *  Copyright (C) 2021  IBM Corporation
>> Copyright (C) 2021  Free Software Foundation, Inc.
>>
>> Or both if you strongly need IBM...
>>
>>> + *  GRUB is free software: you can redistribute it and/or modify
>>> + *  it under the terms of the GNU General Public License as published by
>>> + *  the Free Software Foundation, either version 3 of the License, or
>>> + *  (at your option) any later version.
>>> + *
>>> + *  GRUB is distributed in the hope that it will be useful,
>>> + *  but WITHOUT ANY WARRANTY; without even the implied warranty of
>>> + *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>>> + *  GNU General Public License for more details.
>>> + *
>>> + *  You should have received a copy of the GNU General Public License
>>> + *  along with GRUB.  If not, see .
>>> + *
>>> + *  IBM vTPM support code.
>>> + */
>>> +
>>> +#include 
>>> +#include 
>>> +#include 
>>> +#include 
>>> +#include 
>>> +#include 
>>> +#include 
>>> +
>>> +static grub_ieee1275_ihandle_t grub_tpm_ihandle;
>>> +static grub_uint8_t grub_tpm_version;
>>> +
>>> +static void grub_ieee1275_tpm_init (grub_ieee1275_ihandle_t *tpm_ihandle)
>> You can drop from static functions, variables, etc. names "grub_" prefix.
>>
>>> +{
>>> +  static int init_called = 0;
>>> +
>>> +  if (!init_called) {
>>> +  init_called = 1;
>>> +  grub_ieee1275_open ("/vdevice/vtpm", &grub_tpm_ihandle);
>>> +  }
>> Incorrect formatting. It should be
>>
>> if (!init_called)
>>{
>>  init_called = 1;
>>  grub_ieee1275_open ("/vdevice/vtpm", &grub_tpm_ihandle);
>>}
>>
>> You can always refer to grub-core/kern/efi/sb.c to get formatting
>> examples for various pieces of the GRUB code.
>>
>> Please fix similar issues below too.
>>
>>> +  *tpm_ihandle = grub_tpm_ihandle;
>>> +}
>>> +
>>> +static grub_err_t
>>> +grub_tpm_get_tpm_version (grub_uint8_t *protocol_version)
>>> +{
>>> +  static int version_probed = 0;
>>> +  grub_ieee1275_phandle_t vtpm;
>>> +  char buffer[20];
>>> +  grub_ssize_t buffer_size;
>>> +
>>> +  if (!version_probed) {
>>> + version_probed = 1;
>>> + if (!grub_ieee1275_finddevice ("/vdevice/vtpm", &vtpm) &&
>>> + !grub_ieee1275_get_property (vtpm, "compatible", buffer,
>>> +  sizeof buffer, &buffer_size) &&
>>> + !grub_strcmp (buffer, "IBM,vtpm20")) {
>>> +   grub_tpm_version = 2;
>>> +}
>>> +  }
>>> +  *protocol_version = grub_tpm_version;
>>> +
>>> +  return 0;
>> You ignore this value later.
>>
>> I think the prototype of this function should be following:
>>grub_uint8_t get_tp

RE: UEFI Secureboot not succeeding with Grub 2.06 and later version

2021-07-14 Thread Sayanta Pattanayak


>-Original Message-
>From: Daniel Kiper 
>Sent: Wednesday, July 14, 2021 6:45 PM
>To: Sayanta Pattanayak 
>Cc: grub-devel@gnu.org; nd ; javi...@redhat.com;
>x...@ubuntu.com; pjo...@redhat.com; l...@nuviainc.com
>Subject: Re: UEFI Secureboot not succeeding with Grub 2.06 and later version
>
>On Mon, Jul 12, 2021 at 04:20:56PM +, Sayanta Pattanayak wrote:
>> Hi Daniel,
>>
>> Secureboot worked fine with the change(GRUB_FILE_TYPE_LINUX_KERNEL
>-> GRUB_FILE_TYPE_NONE) you suggested.
>
>Great!
>
>> disk/efi/efidisk.c:531: opening hd0 succeeded
>> partmap/gpt.c:93: Read a valid GPT header
>> partmap/gpt.c:115: GPT entry 0: start=2048, length=40959
>> partmap/gpt.c:115: GPT entry 1: start=43008, length=409599
>> kern/fs.c:56: Detecting ext2...
>> kern/verifiers.c:88: file: /Image type: 0
>> disk/efi/efidisk.c:593: reading 0x40 sectors at the sector 0xcc40 from
>> hd0
>> loader/arm64/linux.c:61: UEFI stub kernel:
>> loader/arm64/linux.c:62: PE/COFF header @ 0040
>> loader/arm64/linux.c:316: kernel file size: 34054136
>> loader/arm64/linux.c:318: kernel numpages: 8314
>> disk/efi/efidisk.c:593: reading 0x40 sectors at the sector 0xcc80 from
>> hd0
>> disk/efi/efidisk.c:593: reading 0x40 sectors at the sector 0xccc0 from
>> hd0 
>> loader/efi/fdt.c:63: allocating 1155 bytes for fdt
>> loader/arm64/linux.c:89: Initrd @ 0xf6e2-0xf6fffa00
>> loader/efi/fdt.c:97: Installed/updated FDT configuration table @
>> 0xf71d Loading driver at 0x000F4D1 EntryPoint=0x000F650EDD8
>> Loading driver at 0x000F4D1 EntryPoint=0x000F650EDD8
>> loader/arm64/linux.c:144: linux command line: 'BOOT_IMAGE=/Image
>> acpi=force
>> console=ttyAMA0,115200 ip=dhcp
>> root=PARTUUID=9c53a91b-e182-4ff1-aeac-6ee2c432ae94 rootwait verbose
>debug'
>> loader/arm64/linux.c:159: starting image 0xfca9a798 EFI stub: Booting
>> Linux Kernel...
>> EFI stub: EFI_RNG_PROTOCOL unavailable EFI stub: UEFI Secure Boot is
>> enabled.
>> EFI stub: Using DTB from configuration table EFI stub: Exiting boot
>> services and installing virtual address map...
>>
>> We understand LoadImage() interface is used in our platform, but if
>> the error scenario is not expected with LoadImage() interface then we
>> need further investigation. We are trying to look into it.
>>
>> What can we infer from the change you suggested and that it worked? Do
>> we need to make certain changes in our platform?
>
>The change which I suggested was just a check for my theory. It is not real 
>fix.
>We have to fix this issue in the GRUB in a different way. This is not your 
>fault.
>When we have a fix we will ask you for some tests.

Thanks for the information. Sure, will look forward for the change and further 
experiments to perform.

>
>Daniel

___
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel