[v2,18/23] efi_loader: implement a pseudo "file delete"

Message ID 20180904074948.18146-19-takahiro.akashi@linaro.org
State New
Headers show
Series
  • subject: fs: fat: extend FAT write operations
Related show

Commit Message

AKASHI Takahiro Sept. 4, 2018, 7:49 a.m.
This patch is necessary to run SCT.efi (UEFI Self-Certification Test).
Returning EFI_SUCCESS can cheat SCT execution.

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

Comments

Alexander Graf Sept. 4, 2018, 9:16 a.m. | #1
On 04.09.18 09:49, AKASHI Takahiro wrote:
> This patch is necessary to run SCT.efi (UEFI Self-Certification Test).
> Returning EFI_SUCCESS can cheat SCT execution.
> 
> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>

How hard would it be to implement a real delete instead?


Alex

> ---
>  lib/efi_loader/efi_file.c | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/lib/efi_loader/efi_file.c b/lib/efi_loader/efi_file.c
> index 6ec98c80227e..12044a0c7196 100644
> --- a/lib/efi_loader/efi_file.c
> +++ b/lib/efi_loader/efi_file.c
> @@ -226,12 +226,20 @@ static efi_status_t EFIAPI efi_file_close(struct efi_file_handle *file)
>  	return EFI_EXIT(file_close(fh));
>  }
>  
> +static efi_status_t EFIAPI efi_file_write(struct efi_file_handle *file,
> +					  efi_uintn_t *buffer_size,
> +					  void *buffer);
> +
>  static efi_status_t EFIAPI efi_file_delete(struct efi_file_handle *file)
>  {
>  	struct file_handle *fh = to_fh(file);
> +	efi_uintn_t size = 0;
>  	EFI_ENTRY("%p", file);
> +
> +	/* TODO: implement real 'delete' */
> +	efi_file_write(file, &size, NULL);
>  	file_close(fh);
> -	return EFI_EXIT(EFI_WARN_DELETE_FAILURE);
> +	return EFI_SUCCESS;
>  }
>  
>  static efi_status_t file_read(struct file_handle *fh, u64 *buffer_size,
>
AKASHI Takahiro Sept. 5, 2018, 2:51 a.m. | #2
On Tue, Sep 04, 2018 at 11:16:38AM +0200, Alexander Graf wrote:
> 
> 
> On 04.09.18 09:49, AKASHI Takahiro wrote:
> > This patch is necessary to run SCT.efi (UEFI Self-Certification Test).
> > Returning EFI_SUCCESS can cheat SCT execution.
> > 
> > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> 
> How hard would it be to implement a real delete instead?

Good question.
The hardest part of implementation, I believe, is about handling
nasty long file names in a directory, in particular, when
directory entries which comprise a single LFN extend across two clusters.
In this case, all the entries must be deleted first, and then go back
to the first one and modify it. Due to a current simple buffering of
cluster (only one cluster buffer a time), some tweak is necessary.
In addition, if there is an already-deleted entry ahead of the given file,
we need further rewind directory entries and update the "new" last valid
one to mark it as so.
Those kind of things would make the implementation a bit complicated
rather than simple as expected.

So unless 'real' delete is a must for anything, I'm lukewarm to
work on it.

# I admit that we don't have to have this patch just if *delete* returns
successfully without doing anything.

Thanks,
-Takahiro AKASHI

> 
> Alex
> 
> > ---
> >  lib/efi_loader/efi_file.c | 10 +++++++++-
> >  1 file changed, 9 insertions(+), 1 deletion(-)
> > 
> > diff --git a/lib/efi_loader/efi_file.c b/lib/efi_loader/efi_file.c
> > index 6ec98c80227e..12044a0c7196 100644
> > --- a/lib/efi_loader/efi_file.c
> > +++ b/lib/efi_loader/efi_file.c
> > @@ -226,12 +226,20 @@ static efi_status_t EFIAPI efi_file_close(struct efi_file_handle *file)
> >  	return EFI_EXIT(file_close(fh));
> >  }
> >  
> > +static efi_status_t EFIAPI efi_file_write(struct efi_file_handle *file,
> > +					  efi_uintn_t *buffer_size,
> > +					  void *buffer);
> > +
> >  static efi_status_t EFIAPI efi_file_delete(struct efi_file_handle *file)
> >  {
> >  	struct file_handle *fh = to_fh(file);
> > +	efi_uintn_t size = 0;
> >  	EFI_ENTRY("%p", file);
> > +
> > +	/* TODO: implement real 'delete' */
> > +	efi_file_write(file, &size, NULL);
> >  	file_close(fh);
> > -	return EFI_EXIT(EFI_WARN_DELETE_FAILURE);
> > +	return EFI_SUCCESS;
> >  }
> >  
> >  static efi_status_t file_read(struct file_handle *fh, u64 *buffer_size,
> >
Alexander Graf Sept. 5, 2018, 8:22 a.m. | #3
On 05.09.18 04:51, AKASHI Takahiro wrote:
> On Tue, Sep 04, 2018 at 11:16:38AM +0200, Alexander Graf wrote:
>>
>>
>> On 04.09.18 09:49, AKASHI Takahiro wrote:
>>> This patch is necessary to run SCT.efi (UEFI Self-Certification Test).
>>> Returning EFI_SUCCESS can cheat SCT execution.
>>>
>>> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
>>
>> How hard would it be to implement a real delete instead?
> 
> Good question.
> The hardest part of implementation, I believe, is about handling
> nasty long file names in a directory, in particular, when
> directory entries which comprise a single LFN extend across two clusters.
> In this case, all the entries must be deleted first, and then go back
> to the first one and modify it. Due to a current simple buffering of
> cluster (only one cluster buffer a time), some tweak is necessary.
> In addition, if there is an already-deleted entry ahead of the given file,
> we need further rewind directory entries and update the "new" last valid
> one to mark it as so.
> Those kind of things would make the implementation a bit complicated
> rather than simple as expected.
> 
> So unless 'real' delete is a must for anything, I'm lukewarm to
> work on it.
> 
> # I admit that we don't have to have this patch just if *delete* returns
> successfully without doing anything.

I think both truncate-to-zero (this patch) and
report-success-when-failure (suggested patch) are hacks that shouldn't
really go upstream that way unfortunately.

The way I read the long file name definitions, I think it should be
perfectly ok to only remove the short file name directory entry. Sure,
we're leaking a few directory entries for the LFN, but I don't think
that's too bad. Any fsck should be able to find those later on...

As for how to delete the entry without rewinding directory entries, you
can just set name[0]=DELETED_FLAG; to indicate that the SFN is deleted, no?


Alex
AKASHI Takahiro Sept. 6, 2018, 2:43 a.m. | #4
On Wed, Sep 05, 2018 at 10:22:07AM +0200, Alexander Graf wrote:
> 
> 
> On 05.09.18 04:51, AKASHI Takahiro wrote:
> > On Tue, Sep 04, 2018 at 11:16:38AM +0200, Alexander Graf wrote:
> >>
> >>
> >> On 04.09.18 09:49, AKASHI Takahiro wrote:
> >>> This patch is necessary to run SCT.efi (UEFI Self-Certification Test).
> >>> Returning EFI_SUCCESS can cheat SCT execution.
> >>>
> >>> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> >>
> >> How hard would it be to implement a real delete instead?
> > 
> > Good question.
> > The hardest part of implementation, I believe, is about handling
> > nasty long file names in a directory, in particular, when
> > directory entries which comprise a single LFN extend across two clusters.
> > In this case, all the entries must be deleted first, and then go back
> > to the first one and modify it. Due to a current simple buffering of
> > cluster (only one cluster buffer a time), some tweak is necessary.
> > In addition, if there is an already-deleted entry ahead of the given file,
> > we need further rewind directory entries and update the "new" last valid
> > one to mark it as so.
> > Those kind of things would make the implementation a bit complicated
> > rather than simple as expected.
> > 
> > So unless 'real' delete is a must for anything, I'm lukewarm to
> > work on it.
> > 
> > # I admit that we don't have to have this patch just if *delete* returns
> > successfully without doing anything.
> 
> I think both truncate-to-zero (this patch) and
> report-success-when-failure (suggested patch) are hacks that shouldn't
> really go upstream that way unfortunately.
> 
> The way I read the long file name definitions, I think it should be
> perfectly ok to only remove the short file name directory entry.

Do you accept such an ugly implementation although it yet complies
with fat specification?

> Sure,
> we're leaking a few directory entries for the LFN, but I don't think
> that's too bad. Any fsck should be able to find those later on...

As I said in my cover-letter, fsck would complain any way.

> As for how to delete the entry without rewinding directory entries, you
> can just set name[0]=DELETED_FLAG; to indicate that the SFN is deleted, no?

Yes if DELETED_FLAG=0x5e and if we ignore the specific case where
name[0] be 0x00, which means the entry is the *first* invalid one.
(again, the latter would not be mandatory in term of compatibility.)

Since I have already had the code, "real" delete will be put in my next
version if nobody has concerns on this simple implementation.

Thanks,
-Takahiro AKASHI

> 
> Alex
Alexander Graf Sept. 6, 2018, 6:09 a.m. | #5
> Am 06.09.2018 um 04:43 schrieb AKASHI Takahiro <takahiro.akashi@linaro.org>:
> 
>> On Wed, Sep 05, 2018 at 10:22:07AM +0200, Alexander Graf wrote:
>> 
>> 
>>> On 05.09.18 04:51, AKASHI Takahiro wrote:
>>>> On Tue, Sep 04, 2018 at 11:16:38AM +0200, Alexander Graf wrote:
>>>> 
>>>> 
>>>>> On 04.09.18 09:49, AKASHI Takahiro wrote:
>>>>> This patch is necessary to run SCT.efi (UEFI Self-Certification Test).
>>>>> Returning EFI_SUCCESS can cheat SCT execution.
>>>>> 
>>>>> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
>>>> 
>>>> How hard would it be to implement a real delete instead?
>>> 
>>> Good question.
>>> The hardest part of implementation, I believe, is about handling
>>> nasty long file names in a directory, in particular, when
>>> directory entries which comprise a single LFN extend across two clusters.
>>> In this case, all the entries must be deleted first, and then go back
>>> to the first one and modify it. Due to a current simple buffering of
>>> cluster (only one cluster buffer a time), some tweak is necessary.
>>> In addition, if there is an already-deleted entry ahead of the given file,
>>> we need further rewind directory entries and update the "new" last valid
>>> one to mark it as so.
>>> Those kind of things would make the implementation a bit complicated
>>> rather than simple as expected.
>>> 
>>> So unless 'real' delete is a must for anything, I'm lukewarm to
>>> work on it.
>>> 
>>> # I admit that we don't have to have this patch just if *delete* returns
>>> successfully without doing anything.
>> 
>> I think both truncate-to-zero (this patch) and
>> report-success-when-failure (suggested patch) are hacks that shouldn't
>> really go upstream that way unfortunately.
>> 
>> The way I read the long file name definitions, I think it should be
>> perfectly ok to only remove the short file name directory entry.
> 
> Do you accept such an ugly implementation although it yet complies
> with fat specification?

It's valid for all intents and purposes, so why not :).

> 
>> Sure,
>> we're leaking a few directory entries for the LFN, but I don't think
>> that's too bad. Any fsck should be able to find those later on...
> 
> As I said in my cover-letter, fsck would complain any way.
> 
>> As for how to delete the entry without rewinding directory entries, you
>> can just set name[0]=DELETED_FLAG; to indicate that the SFN is deleted, no?
> 
> Yes if DELETED_FLAG=0x5e and if we ignore the specific case where
> name[0] be 0x00, which means the entry is the *first* invalid one.
> (again, the latter would not be mandatory in term of compatibility.)
> 
> Since I have already had the code, "real" delete will be put in my next
> version if nobody has concerns on this simple implementation.

Thanks, that would be great! :)

Alex

Patch

diff --git a/lib/efi_loader/efi_file.c b/lib/efi_loader/efi_file.c
index 6ec98c80227e..12044a0c7196 100644
--- a/lib/efi_loader/efi_file.c
+++ b/lib/efi_loader/efi_file.c
@@ -226,12 +226,20 @@  static efi_status_t EFIAPI efi_file_close(struct efi_file_handle *file)
 	return EFI_EXIT(file_close(fh));
 }
 
+static efi_status_t EFIAPI efi_file_write(struct efi_file_handle *file,
+					  efi_uintn_t *buffer_size,
+					  void *buffer);
+
 static efi_status_t EFIAPI efi_file_delete(struct efi_file_handle *file)
 {
 	struct file_handle *fh = to_fh(file);
+	efi_uintn_t size = 0;
 	EFI_ENTRY("%p", file);
+
+	/* TODO: implement real 'delete' */
+	efi_file_write(file, &size, NULL);
 	file_close(fh);
-	return EFI_EXIT(EFI_WARN_DELETE_FAILURE);
+	return EFI_SUCCESS;
 }
 
 static efi_status_t file_read(struct file_handle *fh, u64 *buffer_size,