[U-Boot] [PATCH] efi_loader: disk: install file system protocol to a whole disk

[U-Boot] [PATCH] efi_loader: disk: install file system protocol to a whole disk

[AKASHI Takahiro]

Currently, a whole disk without any partitions is not associated
with EFI_SIMPLE_FILE_SYSTEM_PROTOCOL. So even if it houses FAT
file system, there is a chance that we may not be able to access
it, particularly, when accesses are to be attempted after searching
that protocol against a device handle.

With this patch, EFI_SIMPLE_FILE_SYSTEM_PROTOCOL is installed
to such a disk if part_get_info() shows there is not partition
table installed on it.

Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
---
 lib/efi_loader/efi_disk.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/lib/efi_loader/efi_disk.c b/lib/efi_loader/efi_disk.c
index 7a6b06821a47..548fe667e6f8 100644
--- a/lib/efi_loader/efi_disk.c
+++ b/lib/efi_loader/efi_disk.c
@@ -239,6 +239,7 @@ static efi_status_t efi_disk_add_dev(
 				struct efi_disk_obj **disk)
 {
 	struct efi_disk_obj *diskobj;
+	disk_partition_t info;
 	efi_status_t ret;
 
 	/* Don't add empty devices */
@@ -270,7 +271,8 @@ static efi_status_t efi_disk_add_dev(
 			       diskobj->dp);
 	if (ret != EFI_SUCCESS)
 		return ret;
-	if (part >= 1) {
+	/* partitions or whole disk without partitions */
+	if (part >= 1 || part_get_info(desc, part, &info)) {
 		diskobj->volume = efi_simple_file_system(desc, part,
 							 diskobj->dp);
 		ret = efi_add_protocol(&diskobj->header,
-- 
2.21.0

[U-Boot] [PATCH] efi_loader: disk: install file system protocol to a whole disk

[Mark Kettenis]

> From: AKASHI Takahiro <takahiro.akashi@linaro.org>
> Date: Thu, 22 Aug 2019 17:06:25 +0900
> 
> Currently, a whole disk without any partitions is not associated
> with EFI_SIMPLE_FILE_SYSTEM_PROTOCOL. So even if it houses FAT
> file system, there is a chance that we may not be able to access
> it, particularly, when accesses are to be attempted after searching
> that protocol against a device handle.
> 
> With this patch, EFI_SIMPLE_FILE_SYSTEM_PROTOCOL is installed
> to such a disk if part_get_info() shows there is not partition
> table installed on it.

Do other UEFI implementations support this?

> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> ---
>  lib/efi_loader/efi_disk.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/lib/efi_loader/efi_disk.c b/lib/efi_loader/efi_disk.c
> index 7a6b06821a47..548fe667e6f8 100644
> --- a/lib/efi_loader/efi_disk.c
> +++ b/lib/efi_loader/efi_disk.c
> @@ -239,6 +239,7 @@ static efi_status_t efi_disk_add_dev(
>  				struct efi_disk_obj **disk)
>  {
>  	struct efi_disk_obj *diskobj;
> +	disk_partition_t info;
>  	efi_status_t ret;
>  
>  	/* Don't add empty devices */
> @@ -270,7 +271,8 @@ static efi_status_t efi_disk_add_dev(
>  			       diskobj->dp);
>  	if (ret != EFI_SUCCESS)
>  		return ret;
> -	if (part >= 1) {
> +	/* partitions or whole disk without partitions */
> +	if (part >= 1 || part_get_info(desc, part, &info)) {
>  		diskobj->volume = efi_simple_file_system(desc, part,
>  							 diskobj->dp);
>  		ret = efi_add_protocol(&diskobj->header,
> -- 
> 2.21.0
> 
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> https://lists.denx.de/listinfo/u-boot

[U-Boot] [PATCH] efi_loader: disk: install file system protocol to a whole disk

[Heinrich Schuchardt]

On 8/22/19 11:11 AM, Mark Kettenis wrote:
>> From: AKASHI Takahiro <takahiro.akashi@linaro.org>
>> Date: Thu, 22 Aug 2019 17:06:25 +0900
>>
>> Currently, a whole disk without any partitions is not associated
>> with EFI_SIMPLE_FILE_SYSTEM_PROTOCOL. So even if it houses FAT
>> file system, there is a chance that we may not be able to access
>> it, particularly, when accesses are to be attempted after searching
>> that protocol against a device handle.
>>
>> With this patch, EFI_SIMPLE_FILE_SYSTEM_PROTOCOL is installed
>> to such a disk if part_get_info() shows there is not partition
>> table installed on it.
>
> Do other UEFI implementations support this?

What use cases exist that come without partition table?

You can create an MBR with partition table that is a valid start of a
file system.

So you should first check if a partition table exists. Only if none
exists you can test for a possible file system.

Best regards

Heinrich

>
>> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
>> ---
>>   lib/efi_loader/efi_disk.c | 4 +++-
>>   1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/lib/efi_loader/efi_disk.c b/lib/efi_loader/efi_disk.c
>> index 7a6b06821a47..548fe667e6f8 100644
>> --- a/lib/efi_loader/efi_disk.c
>> +++ b/lib/efi_loader/efi_disk.c
>> @@ -239,6 +239,7 @@ static efi_status_t efi_disk_add_dev(
>>   				struct efi_disk_obj **disk)
>>   {
>>   	struct efi_disk_obj *diskobj;
>> +	disk_partition_t info;
>>   	efi_status_t ret;
>>
>>   	/* Don't add empty devices */
>> @@ -270,7 +271,8 @@ static efi_status_t efi_disk_add_dev(
>>   			       diskobj->dp);
>>   	if (ret != EFI_SUCCESS)
>>   		return ret;
>> -	if (part >= 1) {
>> +	/* partitions or whole disk without partitions */
>> +	if (part >= 1 || part_get_info(desc, part, &info)) {
>>   		diskobj->volume = efi_simple_file_system(desc, part,
>>   							 diskobj->dp);
>>   		ret = efi_add_protocol(&diskobj->header,
>> --
>> 2.21.0
>>
>> _______________________________________________
>> U-Boot mailing list
>> U-Boot at lists.denx.de
>> https://lists.denx.de/listinfo/u-boot
>

[U-Boot] [PATCH] efi_loader: disk: install file system protocol to a whole disk

[AKASHI Takahiro]

On Thu, Aug 22, 2019 at 12:52:41PM +0200, Heinrich Schuchardt wrote:
> On 8/22/19 11:11 AM, Mark Kettenis wrote:
> >>From: AKASHI Takahiro <takahiro.akashi@linaro.org>
> >>Date: Thu, 22 Aug 2019 17:06:25 +0900
> >>
> >>Currently, a whole disk without any partitions is not associated
> >>with EFI_SIMPLE_FILE_SYSTEM_PROTOCOL. So even if it houses FAT
> >>file system, there is a chance that we may not be able to access
> >>it, particularly, when accesses are to be attempted after searching
> >>that protocol against a device handle.
> >>
> >>With this patch, EFI_SIMPLE_FILE_SYSTEM_PROTOCOL is installed
> >>to such a disk if part_get_info() shows there is not partition
> >>table installed on it.
> >
> >Do other UEFI implementations support this?
> 
> What use cases exist that come without partition table?

I didn't find any *explicit* description in UEFI specification
that mandates that any block device should have a partition table.
It may be mandatory for boot(able) disks, but for others?

> You can create an MBR with partition table that is a valid start of a
> file system.

Obviously we can do that, but if this is not a mandatory requirement,
we'd better support no-partitioned cases.

> So you should first check if a partition table exists. Only if none
> exists you can test for a possible file system.

I don't get your point. Are you saying that we should support a file system
for a disk only if it has a file system?
This is not true even for existing partitions as FILE_SYSTEM PROTOCOL
is always installed to every partition whether or not it really
houses a file system under the current implementation.

Thanks,
-Takahiro Akashi


> Best regards
> 
> Heinrich
> 
> >
> >>Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> >>---
> >>  lib/efi_loader/efi_disk.c | 4 +++-
> >>  1 file changed, 3 insertions(+), 1 deletion(-)
> >>
> >>diff --git a/lib/efi_loader/efi_disk.c b/lib/efi_loader/efi_disk.c
> >>index 7a6b06821a47..548fe667e6f8 100644
> >>--- a/lib/efi_loader/efi_disk.c
> >>+++ b/lib/efi_loader/efi_disk.c
> >>@@ -239,6 +239,7 @@ static efi_status_t efi_disk_add_dev(
> >>  				struct efi_disk_obj **disk)
> >>  {
> >>  	struct efi_disk_obj *diskobj;
> >>+	disk_partition_t info;
> >>  	efi_status_t ret;
> >>
> >>  	/* Don't add empty devices */
> >>@@ -270,7 +271,8 @@ static efi_status_t efi_disk_add_dev(
> >>  			       diskobj->dp);
> >>  	if (ret != EFI_SUCCESS)
> >>  		return ret;
> >>-	if (part >= 1) {
> >>+	/* partitions or whole disk without partitions */
> >>+	if (part >= 1 || part_get_info(desc, part, &info)) {
> >>  		diskobj->volume = efi_simple_file_system(desc, part,
> >>  							 diskobj->dp);
> >>  		ret = efi_add_protocol(&diskobj->header,
> >>--
> >>2.21.0
> >>
> >>_______________________________________________
> >>U-Boot mailing list
> >>U-Boot at lists.denx.de
> >>https://lists.denx.de/listinfo/u-boot
> >
> 

[U-Boot] [PATCH] efi_loader: disk: install file system protocol to a whole disk

[AKASHI Takahiro]

Heinrich,

On Fri, Aug 23, 2019 at 09:04:21AM +0900, AKASHI Takahiro wrote:
> On Thu, Aug 22, 2019 at 12:52:41PM +0200, Heinrich Schuchardt wrote:
> > On 8/22/19 11:11 AM, Mark Kettenis wrote:
> > >>From: AKASHI Takahiro <takahiro.akashi@linaro.org>
> > >>Date: Thu, 22 Aug 2019 17:06:25 +0900
> > >>
> > >>Currently, a whole disk without any partitions is not associated
> > >>with EFI_SIMPLE_FILE_SYSTEM_PROTOCOL. So even if it houses FAT
> > >>file system, there is a chance that we may not be able to access
> > >>it, particularly, when accesses are to be attempted after searching
> > >>that protocol against a device handle.
> > >>
> > >>With this patch, EFI_SIMPLE_FILE_SYSTEM_PROTOCOL is installed
> > >>to such a disk if part_get_info() shows there is not partition
> > >>table installed on it.
> > >
> > >Do other UEFI implementations support this?
> > 
> > What use cases exist that come without partition table?
> 
> I didn't find any *explicit* description in UEFI specification
> that mandates that any block device should have a partition table.
> It may be mandatory for boot(able) disks, but for others?
> 
> > You can create an MBR with partition table that is a valid start of a
> > file system.
> 
> Obviously we can do that, but if this is not a mandatory requirement,
> we'd better support no-partitioned cases.

Any further comments?

-Takahiro Akashi

> 
> > So you should first check if a partition table exists. Only if none
> > exists you can test for a possible file system.
> 
> I don't get your point. Are you saying that we should support a file system
> for a disk only if it has a file system?
> This is not true even for existing partitions as FILE_SYSTEM PROTOCOL
> is always installed to every partition whether or not it really
> houses a file system under the current implementation.
> 
> Thanks,
> -Takahiro Akashi
> 
> 
> > Best regards
> > 
> > Heinrich
> > 
> > >
> > >>Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> > >>---
> > >>  lib/efi_loader/efi_disk.c | 4 +++-
> > >>  1 file changed, 3 insertions(+), 1 deletion(-)
> > >>
> > >>diff --git a/lib/efi_loader/efi_disk.c b/lib/efi_loader/efi_disk.c
> > >>index 7a6b06821a47..548fe667e6f8 100644
> > >>--- a/lib/efi_loader/efi_disk.c
> > >>+++ b/lib/efi_loader/efi_disk.c
> > >>@@ -239,6 +239,7 @@ static efi_status_t efi_disk_add_dev(
> > >>  				struct efi_disk_obj **disk)
> > >>  {
> > >>  	struct efi_disk_obj *diskobj;
> > >>+	disk_partition_t info;
> > >>  	efi_status_t ret;
> > >>
> > >>  	/* Don't add empty devices */
> > >>@@ -270,7 +271,8 @@ static efi_status_t efi_disk_add_dev(
> > >>  			       diskobj->dp);
> > >>  	if (ret != EFI_SUCCESS)
> > >>  		return ret;
> > >>-	if (part >= 1) {
> > >>+	/* partitions or whole disk without partitions */
> > >>+	if (part >= 1 || part_get_info(desc, part, &info)) {
> > >>  		diskobj->volume = efi_simple_file_system(desc, part,
> > >>  							 diskobj->dp);
> > >>  		ret = efi_add_protocol(&diskobj->header,
> > >>--
> > >>2.21.0
> > >>
> > >>_______________________________________________
> > >>U-Boot mailing list
> > >>U-Boot at lists.denx.de
> > >>https://lists.denx.de/listinfo/u-boot
> > >
> > 

[U-Boot] [PATCH] efi_loader: disk: install file system protocol to a whole disk

[Heinrich Schuchardt]

On 9/11/19 8:16 AM, AKASHI Takahiro wrote:
> Heinrich,
>
> On Fri, Aug 23, 2019 at 09:04:21AM +0900, AKASHI Takahiro wrote:
>> On Thu, Aug 22, 2019 at 12:52:41PM +0200, Heinrich Schuchardt wrote:
>>> On 8/22/19 11:11 AM, Mark Kettenis wrote:
>>>>> From: AKASHI Takahiro <takahiro.akashi@linaro.org>
>>>>> Date: Thu, 22 Aug 2019 17:06:25 +0900
>>>>>
>>>>> Currently, a whole disk without any partitions is not associated
>>>>> with EFI_SIMPLE_FILE_SYSTEM_PROTOCOL. So even if it houses FAT
>>>>> file system, there is a chance that we may not be able to access
>>>>> it, particularly, when accesses are to be attempted after searching
>>>>> that protocol against a device handle.
>>>>>
>>>>> With this patch, EFI_SIMPLE_FILE_SYSTEM_PROTOCOL is installed
>>>>> to such a disk if part_get_info() shows there is not partition
>>>>> table installed on it.
>>>>
>>>> Do other UEFI implementations support this?
>>>
>>> What use cases exist that come without partition table?
>>
>> I didn't find any *explicit* description in UEFI specification
>> that mandates that any block device should have a partition table.
>> It may be mandatory for boot(able) disks, but for others?
>>
>>> You can create an MBR with partition table that is a valid start of a
>>> file system.
>>
>> Obviously we can do that, but if this is not a mandatory requirement,
>> we'd better support no-partitioned cases.

I did not mean this as a requirement but as a source of errors.

My idea is that could have an MBR which by chance looks like the start
of a file system. When you now expose this non-existent file system the
UEFI client may create havoc.

>
> Any further comments?
>
> -Takahiro Akashi
>
>>
>>> So you should first check if a partition table exists. Only if none
>>> exists you can test for a possible file system.
>>
>> I don't get your point. Are you saying that we should support a file system
>> for a disk only if it has a file system?
>> This is not true even for existing partitions as FILE_SYSTEM PROTOCOL
>> is always installed to every partition whether or not it really
>> houses a file system under the current implementation.

That sounds like a bug. If a partition isn't formatted we would not even
know whether to call the FAT or the EXT4 driver.

Regards

Heinrich

>>
>> Thanks,
>> -Takahiro Akashi
>>
>>
>>> Best regards
>>>
>>> Heinrich
>>>
>>>>
>>>>> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
>>>>> ---
>>>>>   lib/efi_loader/efi_disk.c | 4 +++-
>>>>>   1 file changed, 3 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/lib/efi_loader/efi_disk.c b/lib/efi_loader/efi_disk.c
>>>>> index 7a6b06821a47..548fe667e6f8 100644
>>>>> --- a/lib/efi_loader/efi_disk.c
>>>>> +++ b/lib/efi_loader/efi_disk.c
>>>>> @@ -239,6 +239,7 @@ static efi_status_t efi_disk_add_dev(
>>>>>   				struct efi_disk_obj **disk)
>>>>>   {
>>>>>   	struct efi_disk_obj *diskobj;
>>>>> +	disk_partition_t info;
>>>>>   	efi_status_t ret;
>>>>>
>>>>>   	/* Don't add empty devices */
>>>>> @@ -270,7 +271,8 @@ static efi_status_t efi_disk_add_dev(
>>>>>   			       diskobj->dp);
>>>>>   	if (ret != EFI_SUCCESS)
>>>>>   		return ret;
>>>>> -	if (part >= 1) {
>>>>> +	/* partitions or whole disk without partitions */
>>>>> +	if (part >= 1 || part_get_info(desc, part, &info)) {
>>>>>   		diskobj->volume = efi_simple_file_system(desc, part,
>>>>>   							 diskobj->dp);
>>>>>   		ret = efi_add_protocol(&diskobj->header,
>>>>> --
>>>>> 2.21.0
>>>>>
>>>>> _______________________________________________
>>>>> U-Boot mailing list
>>>>> U-Boot at lists.denx.de
>>>>> https://lists.denx.de/listinfo/u-boot
>>>>
>>>
>

[U-Boot] [PATCH] efi_loader: disk: install file system protocol to a whole disk

[AKASHI Takahiro]

On Wed, Sep 11, 2019 at 07:31:31PM +0200, Heinrich Schuchardt wrote:
> On 9/11/19 8:16 AM, AKASHI Takahiro wrote:
> >Heinrich,
> >
> >On Fri, Aug 23, 2019 at 09:04:21AM +0900, AKASHI Takahiro wrote:
> >>On Thu, Aug 22, 2019 at 12:52:41PM +0200, Heinrich Schuchardt wrote:
> >>>On 8/22/19 11:11 AM, Mark Kettenis wrote:
> >>>>>From: AKASHI Takahiro <takahiro.akashi@linaro.org>
> >>>>>Date: Thu, 22 Aug 2019 17:06:25 +0900
> >>>>>
> >>>>>Currently, a whole disk without any partitions is not associated
> >>>>>with EFI_SIMPLE_FILE_SYSTEM_PROTOCOL. So even if it houses FAT
> >>>>>file system, there is a chance that we may not be able to access
> >>>>>it, particularly, when accesses are to be attempted after searching
> >>>>>that protocol against a device handle.
> >>>>>
> >>>>>With this patch, EFI_SIMPLE_FILE_SYSTEM_PROTOCOL is installed
> >>>>>to such a disk if part_get_info() shows there is not partition
> >>>>>table installed on it.
> >>>>
> >>>>Do other UEFI implementations support this?
> >>>
> >>>What use cases exist that come without partition table?
> >>
> >>I didn't find any *explicit* description in UEFI specification
> >>that mandates that any block device should have a partition table.
> >>It may be mandatory for boot(able) disks, but for others?
> >>
> >>>You can create an MBR with partition table that is a valid start of a
> >>>file system.
> >>
> >>Obviously we can do that, but if this is not a mandatory requirement,
> >>we'd better support no-partitioned cases.
> 
> I did not mean this as a requirement but as a source of errors.
> 
> My idea is that could have an MBR which by chance looks like the start
> of a file system. When you now expose this non-existent file system the
> UEFI client may create havoc.

See below.

> >
> >Any further comments?
> >
> >-Takahiro Akashi
> >
> >>
> >>>So you should first check if a partition table exists. Only if none
> >>>exists you can test for a possible file system.
> >>
> >>I don't get your point. Are you saying that we should support a file system
> >>for a disk only if it has a file system?
> >>This is not true even for existing partitions as FILE_SYSTEM PROTOCOL
> >>is always installed to every partition whether or not it really
> >>houses a file system under the current implementation.
> 
> That sounds like a bug. If a partition isn't formatted we would not even
> know whether to call the FAT or the EXT4 driver.

Yes, the issue does exist in the current implementation.
So a good approach is
1. Check if a partition table is installed or not
2. If yes,
   2-1 If a partition has a file system, install FILE_SYSTEM_PROTOCOL
       to *that* partition.
   2-2 not install FILE_SYSTEM_PROTOCOL to a whole disk
3. If no, and only if a file system exists, install FILE_SYSTEM_PROTOCOL
   to a whole disk

(2.1) and additional check at (3) should be added.

-Takahiro Akashi


> Regards
> 
> Heinrich
> 
> >>
> >>Thanks,
> >>-Takahiro Akashi
> >>
> >>
> >>>Best regards
> >>>
> >>>Heinrich
> >>>
> >>>>
> >>>>>Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> >>>>>---
> >>>>>  lib/efi_loader/efi_disk.c | 4 +++-
> >>>>>  1 file changed, 3 insertions(+), 1 deletion(-)
> >>>>>
> >>>>>diff --git a/lib/efi_loader/efi_disk.c b/lib/efi_loader/efi_disk.c
> >>>>>index 7a6b06821a47..548fe667e6f8 100644
> >>>>>--- a/lib/efi_loader/efi_disk.c
> >>>>>+++ b/lib/efi_loader/efi_disk.c
> >>>>>@@ -239,6 +239,7 @@ static efi_status_t efi_disk_add_dev(
> >>>>>  				struct efi_disk_obj **disk)
> >>>>>  {
> >>>>>  	struct efi_disk_obj *diskobj;
> >>>>>+	disk_partition_t info;
> >>>>>  	efi_status_t ret;
> >>>>>
> >>>>>  	/* Don't add empty devices */
> >>>>>@@ -270,7 +271,8 @@ static efi_status_t efi_disk_add_dev(
> >>>>>  			       diskobj->dp);
> >>>>>  	if (ret != EFI_SUCCESS)
> >>>>>  		return ret;
> >>>>>-	if (part >= 1) {
> >>>>>+	/* partitions or whole disk without partitions */
> >>>>>+	if (part >= 1 || part_get_info(desc, part, &info)) {
> >>>>>  		diskobj->volume = efi_simple_file_system(desc, part,
> >>>>>  							 diskobj->dp);
> >>>>>  		ret = efi_add_protocol(&diskobj->header,
> >>>>>--
> >>>>>2.21.0
> >>>>>
> >>>>>_______________________________________________
> >>>>>U-Boot mailing list
> >>>>>U-Boot at lists.denx.de
> >>>>>https://lists.denx.de/listinfo/u-boot
> >>>>
> >>>
> >
>