[PATCH] efi: Restrict the simple file system protocol to support only FAT

[PATCH] efi: Restrict the simple file system protocol to support only FAT

[Masami Hiramatsu]

Because UEFI specification v2.9, 13.3 File System Format said "The
file system supported by the Extensible Firmware Interface is based
on the FAT file system.", the simple file system protocol might be
better to support only FAT filesystem.

There must be no problem from UEFI application to access only FAT
because ESP must be formatted by FAT32 and the removable media is
FAT12 or FAT16, according to the UEFI spec.

Reported-by: Kazuhiko Sakamoto <sakamoto.kazuhiko@socionext.com>
Signed-off-by: Masami Hiramatsu <masami.hiramatsu@linaro.org>
---
 lib/efi_loader/efi_disk.c |   18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)

diff --git a/lib/efi_loader/efi_disk.c b/lib/efi_loader/efi_disk.c
index 307d5d759b..f69ae6587f 100644
--- a/lib/efi_loader/efi_disk.c
+++ b/lib/efi_loader/efi_disk.c
@@ -318,19 +318,25 @@ efi_fs_from_path(struct efi_device_path *full_path)
 }
 
 /**
- * efi_fs_exists() - check if a partition bears a file system
+ * efi_supported_fs_exists() - check if a partition bears a supported file system
  *
  * @desc:	block device descriptor
  * @part:	partition number
- * Return:	1 if a file system exists on the partition
+ * Return:	1 if a supported file system exists on the partition
  *		0 otherwise
  */
-static int efi_fs_exists(struct blk_desc *desc, int part)
+static int efi_supported_fs_exists(struct blk_desc *desc, int part)
 {
 	if (fs_set_blk_dev_with_part(desc, part))
 		return 0;
 
-	if (fs_get_type() == FS_TYPE_ANY)
+	/*
+	 * Because UEFI specification v2.9, 13.3 File System Format said
+	 * "The file system supported by the Extensible Firmware Interface
+	 * is based on the FAT file system.", the simple file system protocol
+	 * should support only FAT filesystem.
+	 */
+	if (fs_get_type() != FS_TYPE_FAT)
 		return 0;
 
 	fs_close();
@@ -428,10 +434,10 @@ static efi_status_t efi_disk_add_dev(
 
 	/*
 	 * On partitions or whole disks without partitions install the
-	 * simple file system protocol if a file system is available.
+	 * simple file system protocol if a supported file system exists.
 	 */
 	if ((part || desc->part_type == PART_TYPE_UNKNOWN) &&
-	    efi_fs_exists(desc, part)) {
+	    efi_supported_fs_exists(desc, part)) {
 		diskobj->volume = efi_simple_file_system(desc, part,
 							 diskobj->dp);
 		ret = efi_add_protocol(&diskobj->header,

Re: [PATCH] efi: Restrict the simple file system protocol to support only FAT

[Masami Hiramatsu]

+Cc: Grant and Vincent

This will fix some EBBR certification test errors if the target
machine has any partition which is partially (e.g. read only)
supported by U-Boot. UEFI spec doesn't require accessing such
filesystems, but U-Boot provides.

Of course, we can also drop filesystem configs except for FAT
filesystem support for UEFI only platforms. But it doesn't help hybrid
platforms.

Thank you,

2021年6月3日(木) 11:17 Masami Hiramatsu <masami.hiramatsu@linaro.org>:
>
> Because UEFI specification v2.9, 13.3 File System Format said "The
> file system supported by the Extensible Firmware Interface is based
> on the FAT file system.", the simple file system protocol might be
> better to support only FAT filesystem.
>
> There must be no problem from UEFI application to access only FAT
> because ESP must be formatted by FAT32 and the removable media is
> FAT12 or FAT16, according to the UEFI spec.
>
> Reported-by: Kazuhiko Sakamoto <sakamoto.kazuhiko@socionext.com>
> Signed-off-by: Masami Hiramatsu <masami.hiramatsu@linaro.org>
> ---
>  lib/efi_loader/efi_disk.c |   18 ++++++++++++------
>  1 file changed, 12 insertions(+), 6 deletions(-)
>
> diff --git a/lib/efi_loader/efi_disk.c b/lib/efi_loader/efi_disk.c
> index 307d5d759b..f69ae6587f 100644
> --- a/lib/efi_loader/efi_disk.c
> +++ b/lib/efi_loader/efi_disk.c
> @@ -318,19 +318,25 @@ efi_fs_from_path(struct efi_device_path *full_path)
>  }
>
>  /**
> - * efi_fs_exists() - check if a partition bears a file system
> + * efi_supported_fs_exists() - check if a partition bears a supported file system
>   *
>   * @desc:      block device descriptor
>   * @part:      partition number
> - * Return:     1 if a file system exists on the partition
> + * Return:     1 if a supported file system exists on the partition
>   *             0 otherwise
>   */
> -static int efi_fs_exists(struct blk_desc *desc, int part)
> +static int efi_supported_fs_exists(struct blk_desc *desc, int part)
>  {
>         if (fs_set_blk_dev_with_part(desc, part))
>                 return 0;
>
> -       if (fs_get_type() == FS_TYPE_ANY)
> +       /*
> +        * Because UEFI specification v2.9, 13.3 File System Format said
> +        * "The file system supported by the Extensible Firmware Interface
> +        * is based on the FAT file system.", the simple file system protocol
> +        * should support only FAT filesystem.
> +        */
> +       if (fs_get_type() != FS_TYPE_FAT)
>                 return 0;
>
>         fs_close();
> @@ -428,10 +434,10 @@ static efi_status_t efi_disk_add_dev(
>
>         /*
>          * On partitions or whole disks without partitions install the
> -        * simple file system protocol if a file system is available.
> +        * simple file system protocol if a supported file system exists.
>          */
>         if ((part || desc->part_type == PART_TYPE_UNKNOWN) &&
> -           efi_fs_exists(desc, part)) {
> +           efi_supported_fs_exists(desc, part)) {
>                 diskobj->volume = efi_simple_file_system(desc, part,
>                                                          diskobj->dp);
>                 ret = efi_add_protocol(&diskobj->header,
>


-- 
Masami Hiramatsu

Re: [PATCH] efi: Restrict the simple file system protocol to support only FAT

[AKASHI Takahiro]

On Thu, Jun 03, 2021 at 11:17:56AM +0900, Masami Hiramatsu wrote:
> Because UEFI specification v2.9, 13.3 File System Format said "The
> file system supported by the Extensible Firmware Interface is based
> on the FAT file system.", the simple file system protocol might be
> better to support only FAT filesystem.

If I remember correctly, Heinrich rejected the idea a long time ago.

When I posted the commit 867400677cda ("efi_loader: disk: install
FILE_SYSTEM_PROTOCOL only if available"), he insisted that the UEFI
specification does require FAT support but that it doesn't deny
any support for other file systems.
So I had to change the code, allowing FS_TYPE_ANY.

-Takahiro Akashi

> There must be no problem from UEFI application to access only FAT
> because ESP must be formatted by FAT32 and the removable media is
> FAT12 or FAT16, according to the UEFI spec.
> 
> Reported-by: Kazuhiko Sakamoto <sakamoto.kazuhiko@socionext.com>
> Signed-off-by: Masami Hiramatsu <masami.hiramatsu@linaro.org>
> ---
>  lib/efi_loader/efi_disk.c |   18 ++++++++++++------
>  1 file changed, 12 insertions(+), 6 deletions(-)
> 
> diff --git a/lib/efi_loader/efi_disk.c b/lib/efi_loader/efi_disk.c
> index 307d5d759b..f69ae6587f 100644
> --- a/lib/efi_loader/efi_disk.c
> +++ b/lib/efi_loader/efi_disk.c
> @@ -318,19 +318,25 @@ efi_fs_from_path(struct efi_device_path *full_path)
>  }
>  
>  /**
> - * efi_fs_exists() - check if a partition bears a file system
> + * efi_supported_fs_exists() - check if a partition bears a supported file system
>   *
>   * @desc:	block device descriptor
>   * @part:	partition number
> - * Return:	1 if a file system exists on the partition
> + * Return:	1 if a supported file system exists on the partition
>   *		0 otherwise
>   */
> -static int efi_fs_exists(struct blk_desc *desc, int part)
> +static int efi_supported_fs_exists(struct blk_desc *desc, int part)
>  {
>  	if (fs_set_blk_dev_with_part(desc, part))
>  		return 0;
>  
> -	if (fs_get_type() == FS_TYPE_ANY)
> +	/*
> +	 * Because UEFI specification v2.9, 13.3 File System Format said
> +	 * "The file system supported by the Extensible Firmware Interface
> +	 * is based on the FAT file system.", the simple file system protocol
> +	 * should support only FAT filesystem.
> +	 */
> +	if (fs_get_type() != FS_TYPE_FAT)
>  		return 0;
>  
>  	fs_close();
> @@ -428,10 +434,10 @@ static efi_status_t efi_disk_add_dev(
>  
>  	/*
>  	 * On partitions or whole disks without partitions install the
> -	 * simple file system protocol if a file system is available.
> +	 * simple file system protocol if a supported file system exists.
>  	 */
>  	if ((part || desc->part_type == PART_TYPE_UNKNOWN) &&
> -	    efi_fs_exists(desc, part)) {
> +	    efi_supported_fs_exists(desc, part)) {
>  		diskobj->volume = efi_simple_file_system(desc, part,
>  							 diskobj->dp);
>  		ret = efi_add_protocol(&diskobj->header,
> 

Re: [PATCH] efi: Restrict the simple file system protocol to support only FAT

[Masami Hiramatsu]

Hi Akashi-san,

2021年6月3日(木) 11:50 AKASHI Takahiro <takahiro.akashi@linaro.org>:
>
> On Thu, Jun 03, 2021 at 11:17:56AM +0900, Masami Hiramatsu wrote:
> > Because UEFI specification v2.9, 13.3 File System Format said "The
> > file system supported by the Extensible Firmware Interface is based
> > on the FAT file system.", the simple file system protocol might be
> > better to support only FAT filesystem.
>
> If I remember correctly, Heinrich rejected the idea a long time ago.
>
> When I posted the commit 867400677cda ("efi_loader: disk: install
> FILE_SYSTEM_PROTOCOL only if available"), he insisted that the UEFI
> specification does require FAT support but that it doesn't deny
> any support for other file systems.
> So I had to change the code, allowing FS_TYPE_ANY.

Thanks for the good information.
Indeed, the specification doesn't deny any other filesystems.

I think the problem is that the simple file system protocol doesn't
provide any filesystem format information.
EFI_FILE_PROTOCOL.GetInfo() will get the EFI_FILE_SYSTEM_INFO, but it
doesn't include the filesystem format information. Thus, UEFI
application can not know why it can write the file on this volume but
cannot on another volume.

Maybe we can set the ReadOnly field of EFI_FILE_SYSTEM_INFO correctly
if the filesystem doesn't support write operation, and make
EFI_FILE_PROTOCOL.Write() returns EFI_WRITE_PROTECTED. In that case it
may help UEFI application understands what happen.

Thank you,

>
> -Takahiro Akashi
>
> > There must be no problem from UEFI application to access only FAT
> > because ESP must be formatted by FAT32 and the removable media is
> > FAT12 or FAT16, according to the UEFI spec.
> >
> > Reported-by: Kazuhiko Sakamoto <sakamoto.kazuhiko@socionext.com>
> > Signed-off-by: Masami Hiramatsu <masami.hiramatsu@linaro.org>
> > ---
> >  lib/efi_loader/efi_disk.c |   18 ++++++++++++------
> >  1 file changed, 12 insertions(+), 6 deletions(-)
> >
> > diff --git a/lib/efi_loader/efi_disk.c b/lib/efi_loader/efi_disk.c
> > index 307d5d759b..f69ae6587f 100644
> > --- a/lib/efi_loader/efi_disk.c
> > +++ b/lib/efi_loader/efi_disk.c
> > @@ -318,19 +318,25 @@ efi_fs_from_path(struct efi_device_path *full_path)
> >  }
> >
> >  /**
> > - * efi_fs_exists() - check if a partition bears a file system
> > + * efi_supported_fs_exists() - check if a partition bears a supported file system
> >   *
> >   * @desc:    block device descriptor
> >   * @part:    partition number
> > - * Return:   1 if a file system exists on the partition
> > + * Return:   1 if a supported file system exists on the partition
> >   *           0 otherwise
> >   */
> > -static int efi_fs_exists(struct blk_desc *desc, int part)
> > +static int efi_supported_fs_exists(struct blk_desc *desc, int part)
> >  {
> >       if (fs_set_blk_dev_with_part(desc, part))
> >               return 0;
> >
> > -     if (fs_get_type() == FS_TYPE_ANY)
> > +     /*
> > +      * Because UEFI specification v2.9, 13.3 File System Format said
> > +      * "The file system supported by the Extensible Firmware Interface
> > +      * is based on the FAT file system.", the simple file system protocol
> > +      * should support only FAT filesystem.
> > +      */
> > +     if (fs_get_type() != FS_TYPE_FAT)
> >               return 0;
> >
> >       fs_close();
> > @@ -428,10 +434,10 @@ static efi_status_t efi_disk_add_dev(
> >
> >       /*
> >        * On partitions or whole disks without partitions install the
> > -      * simple file system protocol if a file system is available.
> > +      * simple file system protocol if a supported file system exists.
> >        */
> >       if ((part || desc->part_type == PART_TYPE_UNKNOWN) &&
> > -         efi_fs_exists(desc, part)) {
> > +         efi_supported_fs_exists(desc, part)) {
> >               diskobj->volume = efi_simple_file_system(desc, part,
> >                                                        diskobj->dp);
> >               ret = efi_add_protocol(&diskobj->header,
> >



-- 
Masami Hiramatsu

Re: [PATCH] efi: Restrict the simple file system protocol to support only FAT

[Heinrich Schuchardt]

Am 3. Juni 2021 04:17:56 MESZ schrieb Masami Hiramatsu <masami.hiramatsu@linaro.org>:
>Because UEFI specification v2.9, 13.3 File System Format said "The
>file system supported by the Extensible Firmware Interface is based
>on the FAT file system.", the simple file system protocol might be
>better to support only FAT filesystem.
>
>There must be no problem from UEFI application to access only FAT
>because ESP must be formatted by FAT32 and the removable media is
>FAT12 or FAT16, according to the UEFI spec.
>

Does the UEFI spec forbid to access to other file systems?

Which problems were observed?

Best regards

Heinrich


>Reported-by: Kazuhiko Sakamoto <sakamoto.kazuhiko@socionext.com>
>Signed-off-by: Masami Hiramatsu <masami.hiramatsu@linaro.org>
>---
> lib/efi_loader/efi_disk.c |   18 ++++++++++++------
> 1 file changed, 12 insertions(+), 6 deletions(-)
>
>diff --git a/lib/efi_loader/efi_disk.c b/lib/efi_loader/efi_disk.c
>index 307d5d759b..f69ae6587f 100644
>--- a/lib/efi_loader/efi_disk.c
>+++ b/lib/efi_loader/efi_disk.c
>@@ -318,19 +318,25 @@ efi_fs_from_path(struct efi_device_path
>*full_path)
> }
> 
> /**
>- * efi_fs_exists() - check if a partition bears a file system
>+ * efi_supported_fs_exists() - check if a partition bears a supported
>file system
>  *
>  * @desc:	block device descriptor
>  * @part:	partition number
>- * Return:	1 if a file system exists on the partition
>+ * Return:	1 if a supported file system exists on the partition
>  *		0 otherwise
>  */
>-static int efi_fs_exists(struct blk_desc *desc, int part)
>+static int efi_supported_fs_exists(struct blk_desc *desc, int part)
> {
> 	if (fs_set_blk_dev_with_part(desc, part))
> 		return 0;
> 
>-	if (fs_get_type() == FS_TYPE_ANY)
>+	/*
>+	 * Because UEFI specification v2.9, 13.3 File System Format said
>+	 * "The file system supported by the Extensible Firmware Interface
>+	 * is based on the FAT file system.", the simple file system protocol
>+	 * should support only FAT filesystem.
>+	 */
>+	if (fs_get_type() != FS_TYPE_FAT)
> 		return 0;
> 
> 	fs_close();
>@@ -428,10 +434,10 @@ static efi_status_t efi_disk_add_dev(
> 
> 	/*
> 	 * On partitions or whole disks without partitions install the
>-	 * simple file system protocol if a file system is available.
>+	 * simple file system protocol if a supported file system exists.
> 	 */
> 	if ((part || desc->part_type == PART_TYPE_UNKNOWN) &&
>-	    efi_fs_exists(desc, part)) {
>+	    efi_supported_fs_exists(desc, part)) {
> 		diskobj->volume = efi_simple_file_system(desc, part,
> 							 diskobj->dp);
> 		ret = efi_add_protocol(&diskobj->header,

Re: [PATCH] efi: Restrict the simple file system protocol to support only FAT

[Masami Hiramatsu]

Hi Heinrich,

2021年6月3日(木) 13:08 Heinrich Schuchardt <xypron.glpk@gmx.de>:
>
> Am 3. Juni 2021 04:17:56 MESZ schrieb Masami Hiramatsu <masami.hiramatsu@linaro.org>:
> >Because UEFI specification v2.9, 13.3 File System Format said "The
> >file system supported by the Extensible Firmware Interface is based
> >on the FAT file system.", the simple file system protocol might be
> >better to support only FAT filesystem.
> >
> >There must be no problem from UEFI application to access only FAT
> >because ESP must be formatted by FAT32 and the removable media is
> >FAT12 or FAT16, according to the UEFI spec.
> >
>
> Does the UEFI spec forbid to access to other file systems?

No, it does not forbid.

>
> Which problems were observed?

My problem was observed by UEFI SCT (
https://github.com/tianocore/edk2-test/tree/master/uefi-sct ), which
reported errors while testing some volumes formatted by Ext4.

I can fix that with dropping Ext4 support from the U-Boot too. Or
maybe fixed by enabling Ext4 write support. But I thought that is not
a fundamental solution because there are other filesystems which
support read-only (e.g. Btrfs). And UEFI
configuration(CONFIG_EFI_LOADER) doesn't depend on write support.

Of course, the test program itself should check whether the filesystem
is FAT (because UEFI spec only specifies FAT full support, other
filesystems are out-of-spec), but there is no way to determine that
the volume is formatted by FAT (at least in the simple filesystem
protocol). This is reasonable, because UEFI spec expects only FAT.

Thus, I have some ideas except for this fix.
- Check the filesystem driver and only if it supports full operations
(read/write, mkdir etc.), makes it available from UEFI simple file
system protocol (this also checks CONFIG_*_WRITE).
- Set the volume read only if the filesystem driver doesn't support
write and return correct error code. This will give a consistent
filesystem model to the application. (maybe SCT needs to check volume
ReadOnly flag before test it.)

What would you think?

Thank you,


>
> Best regards
>
> Heinrich
>
>
> >Reported-by: Kazuhiko Sakamoto <sakamoto.kazuhiko@socionext.com>
> >Signed-off-by: Masami Hiramatsu <masami.hiramatsu@linaro.org>
> >---
> > lib/efi_loader/efi_disk.c |   18 ++++++++++++------
> > 1 file changed, 12 insertions(+), 6 deletions(-)
> >
> >diff --git a/lib/efi_loader/efi_disk.c b/lib/efi_loader/efi_disk.c
> >index 307d5d759b..f69ae6587f 100644
> >--- a/lib/efi_loader/efi_disk.c
> >+++ b/lib/efi_loader/efi_disk.c
> >@@ -318,19 +318,25 @@ efi_fs_from_path(struct efi_device_path
> >*full_path)
> > }
> >
> > /**
> >- * efi_fs_exists() - check if a partition bears a file system
> >+ * efi_supported_fs_exists() - check if a partition bears a supported
> >file system
> >  *
> >  * @desc:     block device descriptor
> >  * @part:     partition number
> >- * Return:    1 if a file system exists on the partition
> >+ * Return:    1 if a supported file system exists on the partition
> >  *            0 otherwise
> >  */
> >-static int efi_fs_exists(struct blk_desc *desc, int part)
> >+static int efi_supported_fs_exists(struct blk_desc *desc, int part)
> > {
> >       if (fs_set_blk_dev_with_part(desc, part))
> >               return 0;
> >
> >-      if (fs_get_type() == FS_TYPE_ANY)
> >+      /*
> >+       * Because UEFI specification v2.9, 13.3 File System Format said
> >+       * "The file system supported by the Extensible Firmware Interface
> >+       * is based on the FAT file system.", the simple file system protocol
> >+       * should support only FAT filesystem.
> >+       */
> >+      if (fs_get_type() != FS_TYPE_FAT)
> >               return 0;
> >
> >       fs_close();
> >@@ -428,10 +434,10 @@ static efi_status_t efi_disk_add_dev(
> >
> >       /*
> >        * On partitions or whole disks without partitions install the
> >-       * simple file system protocol if a file system is available.
> >+       * simple file system protocol if a supported file system exists.
> >        */
> >       if ((part || desc->part_type == PART_TYPE_UNKNOWN) &&
> >-          efi_fs_exists(desc, part)) {
> >+          efi_supported_fs_exists(desc, part)) {
> >               diskobj->volume = efi_simple_file_system(desc, part,
> >                                                        diskobj->dp);
> >               ret = efi_add_protocol(&diskobj->header,
>


-- 
Masami Hiramatsu

Re: [PATCH] efi: Restrict the simple file system protocol to support only FAT

[Heinrich Schuchardt]

Am 3. Juni 2021 06:57:16 MESZ schrieb Masami Hiramatsu <masami.hiramatsu@linaro.org>:
>Hi Heinrich,
>
>2021年6月3日(木) 13:08 Heinrich Schuchardt <xypron.glpk@gmx.de>:
>>
>> Am 3. Juni 2021 04:17:56 MESZ schrieb Masami Hiramatsu
><masami.hiramatsu@linaro.org>:
>> >Because UEFI specification v2.9, 13.3 File System Format said "The
>> >file system supported by the Extensible Firmware Interface is based
>> >on the FAT file system.", the simple file system protocol might be
>> >better to support only FAT filesystem.
>> >
>> >There must be no problem from UEFI application to access only FAT
>> >because ESP must be formatted by FAT32 and the removable media is
>> >FAT12 or FAT16, according to the UEFI spec.
>> >
>>
>> Does the UEFI spec forbid to access to other file systems?
>
>No, it does not forbid.
>
>>
>> Which problems were observed?
>
>My problem was observed by UEFI SCT (
>https://github.com/tianocore/edk2-test/tree/master/uefi-sct ), which
>reported errors while testing some volumes formatted by Ext4.
>
>I can fix that with dropping Ext4 support from the U-Boot too. Or
>maybe fixed by enabling Ext4 write support. But I thought that is not
>a fundamental solution because there are other filesystems which
>support read-only (e.g. Btrfs). And UEFI
>configuration(CONFIG_EFI_LOADER) doesn't depend on write support.
>
>Of course, the test program itself should check whether the filesystem
>is FAT (because UEFI spec only specifies FAT full support, other
>filesystems are out-of-spec), but there is no way to determine that
>the volume is formatted by FAT (at least in the simple filesystem
>protocol). This is reasonable, because UEFI spec expects only FAT.
>
>Thus, I have some ideas except for this fix.
>- Check the filesystem driver and only if it supports full operations
>(read/write, mkdir etc.), makes it available from UEFI simple file
>system protocol (this also checks CONFIG_*_WRITE).
>- Set the volume read only if the filesystem driver doesn't support
>write and return correct error code. This will give a consistent
>filesystem model to the application. (maybe SCT needs to check volume
>ReadOnly flag before test it.)
>
>What would you think?
>
>Thank you,

For running the SCT I use an image which has only a FAT partition.

At least Debian and Ubuntu do not allow /boot to be on a FAT file system. If we want to boot Linux via the EFI stub without GRUB, we need ext4 support exposed to the EFI sub-system. See Ilias' recent contributions for the EFI_LOAD_FILE2_PROTOCOL for initrd and efidebug. This came in handy for booting via EFI on RISC-V where the initrd= command line parameter is not supported by Linux.

Best regards

Heinrich


>
>
>>
>> Best regards
>>
>> Heinrich
>>
>>
>> >Reported-by: Kazuhiko Sakamoto <sakamoto.kazuhiko@socionext.com>
>> >Signed-off-by: Masami Hiramatsu <masami.hiramatsu@linaro.org>
>> >---
>> > lib/efi_loader/efi_disk.c |   18 ++++++++++++------
>> > 1 file changed, 12 insertions(+), 6 deletions(-)
>> >
>> >diff --git a/lib/efi_loader/efi_disk.c b/lib/efi_loader/efi_disk.c
>> >index 307d5d759b..f69ae6587f 100644
>> >--- a/lib/efi_loader/efi_disk.c
>> >+++ b/lib/efi_loader/efi_disk.c
>> >@@ -318,19 +318,25 @@ efi_fs_from_path(struct efi_device_path
>> >*full_path)
>> > }
>> >
>> > /**
>> >- * efi_fs_exists() - check if a partition bears a file system
>> >+ * efi_supported_fs_exists() - check if a partition bears a
>supported
>> >file system
>> >  *
>> >  * @desc:     block device descriptor
>> >  * @part:     partition number
>> >- * Return:    1 if a file system exists on the partition
>> >+ * Return:    1 if a supported file system exists on the partition
>> >  *            0 otherwise
>> >  */
>> >-static int efi_fs_exists(struct blk_desc *desc, int part)
>> >+static int efi_supported_fs_exists(struct blk_desc *desc, int part)
>> > {
>> >       if (fs_set_blk_dev_with_part(desc, part))
>> >               return 0;
>> >
>> >-      if (fs_get_type() == FS_TYPE_ANY)
>> >+      /*
>> >+       * Because UEFI specification v2.9, 13.3 File System Format
>said
>> >+       * "The file system supported by the Extensible Firmware
>Interface
>> >+       * is based on the FAT file system.", the simple file system
>protocol
>> >+       * should support only FAT filesystem.
>> >+       */
>> >+      if (fs_get_type() != FS_TYPE_FAT)
>> >               return 0;
>> >
>> >       fs_close();
>> >@@ -428,10 +434,10 @@ static efi_status_t efi_disk_add_dev(
>> >
>> >       /*
>> >        * On partitions or whole disks without partitions install
>the
>> >-       * simple file system protocol if a file system is available.
>> >+       * simple file system protocol if a supported file system
>exists.
>> >        */
>> >       if ((part || desc->part_type == PART_TYPE_UNKNOWN) &&
>> >-          efi_fs_exists(desc, part)) {
>> >+          efi_supported_fs_exists(desc, part)) {
>> >               diskobj->volume = efi_simple_file_system(desc, part,
>> >                                                       
>diskobj->dp);
>> >               ret = efi_add_protocol(&diskobj->header,
>>

Re: [PATCH] efi: Restrict the simple file system protocol to support only FAT

[Masami Hiramatsu]

Hi Heinrich,

2021年6月3日(木) 14:15 Heinrich Schuchardt <xypron.glpk@gmx.de>:
>
> Am 3. Juni 2021 06:57:16 MESZ schrieb Masami Hiramatsu <masami.hiramatsu@linaro.org>:
> >Hi Heinrich,
> >
> >2021年6月3日(木) 13:08 Heinrich Schuchardt <xypron.glpk@gmx.de>:
> >>
> >> Am 3. Juni 2021 04:17:56 MESZ schrieb Masami Hiramatsu
> ><masami.hiramatsu@linaro.org>:
> >> >Because UEFI specification v2.9, 13.3 File System Format said "The
> >> >file system supported by the Extensible Firmware Interface is based
> >> >on the FAT file system.", the simple file system protocol might be
> >> >better to support only FAT filesystem.
> >> >
> >> >There must be no problem from UEFI application to access only FAT
> >> >because ESP must be formatted by FAT32 and the removable media is
> >> >FAT12 or FAT16, according to the UEFI spec.
> >> >
> >>
> >> Does the UEFI spec forbid to access to other file systems?
> >
> >No, it does not forbid.
> >
> >>
> >> Which problems were observed?
> >
> >My problem was observed by UEFI SCT (
> >https://github.com/tianocore/edk2-test/tree/master/uefi-sct ), which
> >reported errors while testing some volumes formatted by Ext4.
> >
> >I can fix that with dropping Ext4 support from the U-Boot too. Or
> >maybe fixed by enabling Ext4 write support. But I thought that is not
> >a fundamental solution because there are other filesystems which
> >support read-only (e.g. Btrfs). And UEFI
> >configuration(CONFIG_EFI_LOADER) doesn't depend on write support.
> >
> >Of course, the test program itself should check whether the filesystem
> >is FAT (because UEFI spec only specifies FAT full support, other
> >filesystems are out-of-spec), but there is no way to determine that
> >the volume is formatted by FAT (at least in the simple filesystem
> >protocol). This is reasonable, because UEFI spec expects only FAT.
> >
> >Thus, I have some ideas except for this fix.
> >- Check the filesystem driver and only if it supports full operations
> >(read/write, mkdir etc.), makes it available from UEFI simple file
> >system protocol (this also checks CONFIG_*_WRITE).
> >- Set the volume read only if the filesystem driver doesn't support
> >write and return correct error code. This will give a consistent
> >filesystem model to the application. (maybe SCT needs to check volume
> >ReadOnly flag before test it.)
> >
> >What would you think?
> >
> >Thank you,
>
> For running the SCT I use an image which has only a FAT partition.

That depends on the device configuration. My platform (DeveloperBox)
is something like PC, which has not only USB, but eMMC, SATA, NVMe. Of
course I can just disable CONFIG_EXT4 from U-Boot for SCT, but I don't
like erasing all the partitions on which I have installed debian...

>
> At least Debian and Ubuntu do not allow /boot to be on a FAT file system. If we want to boot Linux via the EFI stub without GRUB, we need ext4 support exposed to the EFI sub-system. See Ilias' recent contributions for the EFI_LOAD_FILE2_PROTOCOL for initrd and efidebug. This came in handy for booting via EFI on RISC-V where the initrd= command line parameter is not supported by Linux.

IMHO, such dependency is out of UEFI spec. That means Debian/Ubuntu
doesn't follow the UEFI spec. (but as far as I know, those install ESP
on the disk and install GRUB efi application for boot)
And yes, EFI_LOAD_FILE2_PROTOCOL needs to load initrd from somewhere
(I'm usually put it on the ESP). But, if the EFI_LOAD_FILE2_PROTOCOL
*requires* to access ext4 partition, I think that is not supported by
UEFI spec.

Anyway, I agree that denying access to non-FAT partitions is too
restricted. What about my other ideas? If the volume is set to
ReadOnly, that is good for both of the SCT and the
EFI_LOAD_FILE2_PROTOCOL.


Thank you,

>
> Best regards
>
> Heinrich
>
>
> >
> >
> >>
> >> Best regards
> >>
> >> Heinrich
> >>
> >>
> >> >Reported-by: Kazuhiko Sakamoto <sakamoto.kazuhiko@socionext.com>
> >> >Signed-off-by: Masami Hiramatsu <masami.hiramatsu@linaro.org>
> >> >---
> >> > lib/efi_loader/efi_disk.c |   18 ++++++++++++------
> >> > 1 file changed, 12 insertions(+), 6 deletions(-)
> >> >
> >> >diff --git a/lib/efi_loader/efi_disk.c b/lib/efi_loader/efi_disk.c
> >> >index 307d5d759b..f69ae6587f 100644
> >> >--- a/lib/efi_loader/efi_disk.c
> >> >+++ b/lib/efi_loader/efi_disk.c
> >> >@@ -318,19 +318,25 @@ efi_fs_from_path(struct efi_device_path
> >> >*full_path)
> >> > }
> >> >
> >> > /**
> >> >- * efi_fs_exists() - check if a partition bears a file system
> >> >+ * efi_supported_fs_exists() - check if a partition bears a
> >supported
> >> >file system
> >> >  *
> >> >  * @desc:     block device descriptor
> >> >  * @part:     partition number
> >> >- * Return:    1 if a file system exists on the partition
> >> >+ * Return:    1 if a supported file system exists on the partition
> >> >  *            0 otherwise
> >> >  */
> >> >-static int efi_fs_exists(struct blk_desc *desc, int part)
> >> >+static int efi_supported_fs_exists(struct blk_desc *desc, int part)
> >> > {
> >> >       if (fs_set_blk_dev_with_part(desc, part))
> >> >               return 0;
> >> >
> >> >-      if (fs_get_type() == FS_TYPE_ANY)
> >> >+      /*
> >> >+       * Because UEFI specification v2.9, 13.3 File System Format
> >said
> >> >+       * "The file system supported by the Extensible Firmware
> >Interface
> >> >+       * is based on the FAT file system.", the simple file system
> >protocol
> >> >+       * should support only FAT filesystem.
> >> >+       */
> >> >+      if (fs_get_type() != FS_TYPE_FAT)
> >> >               return 0;
> >> >
> >> >       fs_close();
> >> >@@ -428,10 +434,10 @@ static efi_status_t efi_disk_add_dev(
> >> >
> >> >       /*
> >> >        * On partitions or whole disks without partitions install
> >the
> >> >-       * simple file system protocol if a file system is available.
> >> >+       * simple file system protocol if a supported file system
> >exists.
> >> >        */
> >> >       if ((part || desc->part_type == PART_TYPE_UNKNOWN) &&
> >> >-          efi_fs_exists(desc, part)) {
> >> >+          efi_supported_fs_exists(desc, part)) {
> >> >               diskobj->volume = efi_simple_file_system(desc, part,
> >> >
> >diskobj->dp);
> >> >               ret = efi_add_protocol(&diskobj->header,
> >>
>


-- 
Masami Hiramatsu

Re: [PATCH] efi: Restrict the simple file system protocol to support only FAT

[Heinrich Schuchardt]

On 6/3/21 7:39 AM, Masami Hiramatsu wrote:
> Hi Heinrich,
>
> 2021年6月3日(木) 14:15 Heinrich Schuchardt <xypron.glpk@gmx.de>:
>>
>> Am 3. Juni 2021 06:57:16 MESZ schrieb Masami Hiramatsu <masami.hiramatsu@linaro.org>:
>>> Hi Heinrich,
>>>
>>> 2021年6月3日(木) 13:08 Heinrich Schuchardt <xypron.glpk@gmx.de>:
>>>>
>>>> Am 3. Juni 2021 04:17:56 MESZ schrieb Masami Hiramatsu
>>> <masami.hiramatsu@linaro.org>:
>>>>> Because UEFI specification v2.9, 13.3 File System Format said "The
>>>>> file system supported by the Extensible Firmware Interface is based
>>>>> on the FAT file system.", the simple file system protocol might be
>>>>> better to support only FAT filesystem.
>>>>>
>>>>> There must be no problem from UEFI application to access only FAT
>>>>> because ESP must be formatted by FAT32 and the removable media is
>>>>> FAT12 or FAT16, according to the UEFI spec.
>>>>>
>>>>
>>>> Does the UEFI spec forbid to access to other file systems?
>>>
>>> No, it does not forbid.
>>>
>>>>
>>>> Which problems were observed?
>>>
>>> My problem was observed by UEFI SCT (
>>> https://github.com/tianocore/edk2-test/tree/master/uefi-sct ), which
>>> reported errors while testing some volumes formatted by Ext4.
>>>
>>> I can fix that with dropping Ext4 support from the U-Boot too. Or
>>> maybe fixed by enabling Ext4 write support. But I thought that is not
>>> a fundamental solution because there are other filesystems which
>>> support read-only (e.g. Btrfs). And UEFI
>>> configuration(CONFIG_EFI_LOADER) doesn't depend on write support.
>>>
>>> Of course, the test program itself should check whether the filesystem
>>> is FAT (because UEFI spec only specifies FAT full support, other
>>> filesystems are out-of-spec), but there is no way to determine that
>>> the volume is formatted by FAT (at least in the simple filesystem
>>> protocol). This is reasonable, because UEFI spec expects only FAT.
>>>
>>> Thus, I have some ideas except for this fix.
>>> - Check the filesystem driver and only if it supports full operations
>>> (read/write, mkdir etc.), makes it available from UEFI simple file
>>> system protocol (this also checks CONFIG_*_WRITE).
>>> - Set the volume read only if the filesystem driver doesn't support
>>> write and return correct error code. This will give a consistent
>>> filesystem model to the application. (maybe SCT needs to check volume
>>> ReadOnly flag before test it.)
>>>
>>> What would you think?
>>>
>>> Thank you,
>>
>> For running the SCT I use an image which has only a FAT partition.
>
> That depends on the device configuration. My platform (DeveloperBox)
> is something like PC, which has not only USB, but eMMC, SATA, NVMe. Of
> course I can just disable CONFIG_EXT4 from U-Boot for SCT, but I don't
> like erasing all the partitions on which I have installed debian...
>
>>
>> At least Debian and Ubuntu do not allow /boot to be on a FAT file system. If we want to boot Linux via the EFI stub without GRUB, we need ext4 support exposed to the EFI sub-system. See Ilias' recent contributions for the EFI_LOAD_FILE2_PROTOCOL for initrd and efidebug. This came in handy for booting via EFI on RISC-V where the initrd= command line parameter is not supported by Linux.
>
> IMHO, such dependency is out of UEFI spec. That means Debian/Ubuntu
> doesn't follow the UEFI spec. (but as far as I know, those install ESP
> on the disk and install GRUB efi application for boot)
> And yes, EFI_LOAD_FILE2_PROTOCOL needs to load initrd from somewhere
> (I'm usually put it on the ESP). But, if the EFI_LOAD_FILE2_PROTOCOL
> *requires* to access ext4 partition, I think that is not supported by
> UEFI spec.
>
> Anyway, I agree that denying access to non-FAT partitions is too
> restricted. What about my other ideas? If the volume is set to
> ReadOnly, that is good for both of the SCT and the
> EFI_LOAD_FILE2_PROTOCOL.
>

If a volume or file is read only the UEFI spec requires to report this
in EFI_FILE_PROTOCOL.GetInfo().

On partition level we have the following deficiencies in U-Boot:

* we cannot (re-)mount a partition read-only
* we cannot determine if a partition is read-only
* we can neither read nor write the volume label
* we cannot determine the free space.

The current implementation of the FAT driver does not support reading
attributes and dates. I have started looking into what is needed in the
FAT driver.
https://github.com/xypron/u-boot-patches/blob/bfe483ed97978678b124f8fe579682aab6e3e9d8/patch-efi-next.sh#L91
But it is not ready for submission.

Best regards

Heinrich

>
> Thank you,
>
>>
>> Best regards
>>
>> Heinrich
>>
>>
>>>
>>>
>>>>
>>>> Best regards
>>>>
>>>> Heinrich
>>>>
>>>>
>>>>> Reported-by: Kazuhiko Sakamoto <sakamoto.kazuhiko@socionext.com>
>>>>> Signed-off-by: Masami Hiramatsu <masami.hiramatsu@linaro.org>
>>>>> ---
>>>>> lib/efi_loader/efi_disk.c |   18 ++++++++++++------
>>>>> 1 file changed, 12 insertions(+), 6 deletions(-)
>>>>>
>>>>> diff --git a/lib/efi_loader/efi_disk.c b/lib/efi_loader/efi_disk.c
>>>>> index 307d5d759b..f69ae6587f 100644
>>>>> --- a/lib/efi_loader/efi_disk.c
>>>>> +++ b/lib/efi_loader/efi_disk.c
>>>>> @@ -318,19 +318,25 @@ efi_fs_from_path(struct efi_device_path
>>>>> *full_path)
>>>>> }
>>>>>
>>>>> /**
>>>>> - * efi_fs_exists() - check if a partition bears a file system
>>>>> + * efi_supported_fs_exists() - check if a partition bears a
>>> supported
>>>>> file system
>>>>>   *
>>>>>   * @desc:     block device descriptor
>>>>>   * @part:     partition number
>>>>> - * Return:    1 if a file system exists on the partition
>>>>> + * Return:    1 if a supported file system exists on the partition
>>>>>   *            0 otherwise
>>>>>   */
>>>>> -static int efi_fs_exists(struct blk_desc *desc, int part)
>>>>> +static int efi_supported_fs_exists(struct blk_desc *desc, int part)
>>>>> {
>>>>>        if (fs_set_blk_dev_with_part(desc, part))
>>>>>                return 0;
>>>>>
>>>>> -      if (fs_get_type() == FS_TYPE_ANY)
>>>>> +      /*
>>>>> +       * Because UEFI specification v2.9, 13.3 File System Format
>>> said
>>>>> +       * "The file system supported by the Extensible Firmware
>>> Interface
>>>>> +       * is based on the FAT file system.", the simple file system
>>> protocol
>>>>> +       * should support only FAT filesystem.
>>>>> +       */
>>>>> +      if (fs_get_type() != FS_TYPE_FAT)
>>>>>                return 0;
>>>>>
>>>>>        fs_close();
>>>>> @@ -428,10 +434,10 @@ static efi_status_t efi_disk_add_dev(
>>>>>
>>>>>        /*
>>>>>         * On partitions or whole disks without partitions install
>>> the
>>>>> -       * simple file system protocol if a file system is available.
>>>>> +       * simple file system protocol if a supported file system
>>> exists.
>>>>>         */
>>>>>        if ((part || desc->part_type == PART_TYPE_UNKNOWN) &&
>>>>> -          efi_fs_exists(desc, part)) {
>>>>> +          efi_supported_fs_exists(desc, part)) {
>>>>>                diskobj->volume = efi_simple_file_system(desc, part,
>>>>>
>>> diskobj->dp);
>>>>>                ret = efi_add_protocol(&diskobj->header,
>>>>
>>
>
>

Re: [PATCH] efi: Restrict the simple file system protocol to support only FAT

[Ilias Apalodimas]

[...]
> >
> > At least Debian and Ubuntu do not allow /boot to be on a FAT file system. If we want to boot Linux via the EFI stub without GRUB, we need ext4 support exposed to the EFI sub-system. See Ilias' recent contributions for the EFI_LOAD_FILE2_PROTOCOL for initrd and efidebug. This came in handy for booting via EFI on RISC-V where the initrd= command line parameter is not supported by Linux.
> 
> IMHO, such dependency is out of UEFI spec. That means Debian/Ubuntu
> doesn't follow the UEFI spec. (but as far as I know, those install ESP
> on the disk and install GRUB efi application for boot)
> And yes, EFI_LOAD_FILE2_PROTOCOL needs to load initrd from somewhere
> (I'm usually put it on the ESP). But, if the EFI_LOAD_FILE2_PROTOCOL
> *requires* to access ext4 partition, I think that is not supported by
> UEFI spec.

One of the advantages in using EFI_LOAD_FILE2_PROTOCOL is that you can load
it from *any* file system the firmware has access to. The only thing the
kernel does is provide a buffer big enough to fit in the initrd.  The
firmware is free to locate the file and copy it in that memory however it
sees fit.

Cheers
/Ilias
> 
> Anyway, I agree that denying access to non-FAT partitions is too
> restricted. What about my other ideas? If the volume is set to
> ReadOnly, that is good for both of the SCT and the
> EFI_LOAD_FILE2_PROTOCOL.
> 
> 
> Thank you,
> 
> >
> > Best regards
> >
> > Heinrich
> >
> >

Re: [PATCH] efi: Restrict the simple file system protocol to support only FAT

[Masami Hiramatsu]

Hi Ilias,

2021年6月3日(木) 15:25 Ilias Apalodimas <ilias.apalodimas@linaro.org>:
>
> [...]
> > >
> > > At least Debian and Ubuntu do not allow /boot to be on a FAT file system. If we want to boot Linux via the EFI stub without GRUB, we need ext4 support exposed to the EFI sub-system. See Ilias' recent contributions for the EFI_LOAD_FILE2_PROTOCOL for initrd and efidebug. This came in handy for booting via EFI on RISC-V where the initrd= command line parameter is not supported by Linux.
> >
> > IMHO, such dependency is out of UEFI spec. That means Debian/Ubuntu
> > doesn't follow the UEFI spec. (but as far as I know, those install ESP
> > on the disk and install GRUB efi application for boot)
> > And yes, EFI_LOAD_FILE2_PROTOCOL needs to load initrd from somewhere
> > (I'm usually put it on the ESP). But, if the EFI_LOAD_FILE2_PROTOCOL
> > *requires* to access ext4 partition, I think that is not supported by
> > UEFI spec.
>
> One of the advantages in using EFI_LOAD_FILE2_PROTOCOL is that you can load
> it from *any* file system the firmware has access to. The only thing the
> kernel does is provide a buffer big enough to fit in the initrd.  The
> firmware is free to locate the file and copy it in that memory however it
> sees fit.

Ah, I got it. Yes, EFI_LOAD_FILE2_PROTOCOL doesn't depend on the
EFI_SIMPLE_FILE_SYSTEM_PROTOCOL. Thus it should be able to load
the file from where the U-Boot can access. However, since current implementation
depends on the EFI_SIMPLE_FILE_SYSTEM_PROTOCOL, my patch limits
the ability...

Thank you,

>
> Cheers
> /Ilias
> >
> > Anyway, I agree that denying access to non-FAT partitions is too
> > restricted. What about my other ideas? If the volume is set to
> > ReadOnly, that is good for both of the SCT and the
> > EFI_LOAD_FILE2_PROTOCOL.
> >
> >
> > Thank you,
> >
> > >
> > > Best regards
> > >
> > > Heinrich
> > >
> > >



-- 
Masami Hiramatsu

Re: [PATCH] efi: Restrict the simple file system protocol to support only FAT

[Ilias Apalodimas]

On Thu, Jun 03, 2021 at 03:36:38PM +0900, Masami Hiramatsu wrote:
> Hi Ilias,
> 
> 2021年6月3日(木) 15:25 Ilias Apalodimas <ilias.apalodimas@linaro.org>:
> >
> > [...]
> > > >
> > > > At least Debian and Ubuntu do not allow /boot to be on a FAT file system. If we want to boot Linux via the EFI stub without GRUB, we need ext4 support exposed to the EFI sub-system. See Ilias' recent contributions for the EFI_LOAD_FILE2_PROTOCOL for initrd and efidebug. This came in handy for booting via EFI on RISC-V where the initrd= command line parameter is not supported by Linux.
> > >
> > > IMHO, such dependency is out of UEFI spec. That means Debian/Ubuntu
> > > doesn't follow the UEFI spec. (but as far as I know, those install ESP
> > > on the disk and install GRUB efi application for boot)
> > > And yes, EFI_LOAD_FILE2_PROTOCOL needs to load initrd from somewhere
> > > (I'm usually put it on the ESP). But, if the EFI_LOAD_FILE2_PROTOCOL
> > > *requires* to access ext4 partition, I think that is not supported by
> > > UEFI spec.
> >
> > One of the advantages in using EFI_LOAD_FILE2_PROTOCOL is that you can load
> > it from *any* file system the firmware has access to. The only thing the
> > kernel does is provide a buffer big enough to fit in the initrd.  The
> > firmware is free to locate the file and copy it in that memory however it
> > sees fit.
> 
> Ah, I got it. Yes, EFI_LOAD_FILE2_PROTOCOL doesn't depend on the
> EFI_SIMPLE_FILE_SYSTEM_PROTOCOL. Thus it should be able to load
> the file from where the U-Boot can access. However, since current implementation
> depends on the EFI_SIMPLE_FILE_SYSTEM_PROTOCOL, my patch limits
> the ability...
> 

Yea that changed recently. On the first version, I was using u-boot
internal functions to locate and load the file.  When we decided to store a
device path on Boot#### options, in order to locate the initrd, we started
using the EFI APIs to load it. You can check commit 53f6a5aa8626 for more
details.

Cheers
/Ilias
> Thank you,
> 
> >
> > Cheers
> > /Ilias
> > >
> > > Anyway, I agree that denying access to non-FAT partitions is too
> > > restricted. What about my other ideas? If the volume is set to
> > > ReadOnly, that is good for both of the SCT and the
> > > EFI_LOAD_FILE2_PROTOCOL.
> > >
> > >
> > > Thank you,
> > >
> > > >
> > > > Best regards
> > > >
> > > > Heinrich
> > > >
> > > >
> 
> 
> 
> -- 
> Masami Hiramatsu

Re: [PATCH] efi: Restrict the simple file system protocol to support only FAT

[Masami Hiramatsu]

Hi Heinrich,

2021年6月3日(木) 15:14 Heinrich Schuchardt <xypron.glpk@gmx.de>:
[..]
>
> If a volume or file is read only the UEFI spec requires to report this
> in EFI_FILE_PROTOCOL.GetInfo().

Yes.

>
> On partition level we have the following deficiencies in U-Boot:
>
> * we cannot (re-)mount a partition read-only
> * we cannot determine if a partition is read-only

Hmm, but I think the filesystem driver itself defined it in fstype_info.
Thus we can check it as below.
fsinfo = fs_get_info(fs_get_type());
if (fsinfo->write == fs_write_unsupported)
  // then set the volume readonly

BTW, it seems fsinfo->opendir and readdir are also required for
EFI_FILE_PROTOCOL, unless it, UEFI application can not follow the
directory tree. Hmm.

> * we can neither read nor write the volume label
> * we cannot determine the free space.
>
> The current implementation of the FAT driver does not support reading
> attributes and dates. I have started looking into what is needed in the
> FAT driver.
> https://github.com/xypron/u-boot-patches/blob/bfe483ed97978678b124f8fe579682aab6e3e9d8/patch-efi-next.sh#L91
> But it is not ready for submission.

Ah, that's great! I would like to test it.

Thank you,


-- 
Masami Hiramatsu