[PATCH] efi/libstub: Fix path separator regression

[PATCH] efi/libstub: Fix path separator regression

[Philipp Fent]

Commit 9302c1bb8e47 ("efi/libstub: Rewrite file I/O routine") introduced a
regression that made a couple of (badly configured) systems fail to
boot [1]: Until 5.6, we silently accepted Unix-style file separators in
EFI paths, which might violate the EFI standard, but are an easy to make
mistake. This fix restores the pre-5.7 behaviour.

[1] https://bbs.archlinux.org/viewtopic.php?id=256273

Signed-off-by: Philipp Fent <fent@in.tum.de>
---
 drivers/firmware/efi/libstub/file.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/firmware/efi/libstub/file.c b/drivers/firmware/efi/libstub/file.c
index 2005e33b33d5..f8a28a6e0bde 100644
--- a/drivers/firmware/efi/libstub/file.c
+++ b/drivers/firmware/efi/libstub/file.c
@@ -102,11 +102,21 @@ static int find_file_option(const efi_char16_t *cmdline, int cmdline_len,
 	if (!found)
 		return 0;
 
+	/* Skip any leading slashes */
+	while (cmdline[i] == L'/' || cmdline[i] == L'\\')
+		i++;
+
 	while (--result_len > 0 && i < cmdline_len) {
 		if (cmdline[i] == L'\0' ||
 		    cmdline[i] == L'\n' ||
 		    cmdline[i] == L' ')
 			break;
+		/* Replace UNIX dir separators with EFI standard separators */
+		if (cmdline[i] == L'/') {
+			*result++ = L'\\';
+			i++;
+			continue;
+		}
 		*result++ = cmdline[i++];
 	}
 	*result = L'\0';
-- 
2.27.0

Re: [PATCH] efi/libstub: Fix path separator regression

[Ard Biesheuvel]

On Mon, 15 Jun 2020 at 13:58, Philipp Fent <fent@in.tum.de> wrote:
>
> Commit 9302c1bb8e47 ("efi/libstub: Rewrite file I/O routine") introduced a
> regression that made a couple of (badly configured) systems fail to
> boot [1]: Until 5.6, we silently accepted Unix-style file separators in
> EFI paths, which might violate the EFI standard, but are an easy to make
> mistake. This fix restores the pre-5.7 behaviour.
>
> [1] https://bbs.archlinux.org/viewtopic.php?id=256273
>
> Signed-off-by: Philipp Fent <fent@in.tum.de>

Apologies for the breakage. I will queue this as a fix.


> ---
>  drivers/firmware/efi/libstub/file.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
>
> diff --git a/drivers/firmware/efi/libstub/file.c b/drivers/firmware/efi/libstub/file.c
> index 2005e33b33d5..f8a28a6e0bde 100644
> --- a/drivers/firmware/efi/libstub/file.c
> +++ b/drivers/firmware/efi/libstub/file.c
> @@ -102,11 +102,21 @@ static int find_file_option(const efi_char16_t *cmdline, int cmdline_len,
>         if (!found)
>                 return 0;
>
> +       /* Skip any leading slashes */
> +       while (cmdline[i] == L'/' || cmdline[i] == L'\\')
> +               i++;
> +
>         while (--result_len > 0 && i < cmdline_len) {
>                 if (cmdline[i] == L'\0' ||
>                     cmdline[i] == L'\n' ||
>                     cmdline[i] == L' ')
>                         break;
> +               /* Replace UNIX dir separators with EFI standard separators */
> +               if (cmdline[i] == L'/') {
> +                       *result++ = L'\\';
> +                       i++;
> +                       continue;
> +               }

Any objections if I change this to


---%<---
efi_char16_t c = cmdline[i++];

if (c == L'\0' || c == L'\n' || c == L' ')
  break;
else if (c == L'/')
  /* Replace UNIX dir separators with EFI standard ones */
  *result++ = L'\\';
else
  *result++ = c;
---%<---


(no need to resend)


>                 *result++ = cmdline[i++];
>         }
>         *result = L'\0';
> --
> 2.27.0
>

Re: [PATCH] efi/libstub: Fix path separator regression

[Philipp Fent]

On 15.06.20 14:15, Ard Biesheuvel wrote:
>> ---
>>  drivers/firmware/efi/libstub/file.c | 10 ++++++++++
>>  1 file changed, 10 insertions(+)
>>
>> diff --git a/drivers/firmware/efi/libstub/file.c b/drivers/firmware/efi/libstub/file.c
>> index 2005e33b33d5..f8a28a6e0bde 100644
>> --- a/drivers/firmware/efi/libstub/file.c
>> +++ b/drivers/firmware/efi/libstub/file.c
>> @@ -102,11 +102,21 @@ static int find_file_option(const efi_char16_t *cmdline, int cmdline_len,
>>         if (!found)
>>                 return 0;
>>
>> +       /* Skip any leading slashes */
>> +       while (cmdline[i] == L'/' || cmdline[i] == L'\\')
>> +               i++;
>> +
>>         while (--result_len > 0 && i < cmdline_len) {
>>                 if (cmdline[i] == L'\0' ||
>>                     cmdline[i] == L'\n' ||
>>                     cmdline[i] == L' ')
>>                         break;
>> +               /* Replace UNIX dir separators with EFI standard separators */
>> +               if (cmdline[i] == L'/') {
>> +                       *result++ = L'\\';
>> +                       i++;
>> +                       continue;
>> +               }
> 
> Any objections if I change this to
> 
> 
> ---%<---
> efi_char16_t c = cmdline[i++];
> 
> if (c == L'\0' || c == L'\n' || c == L' ')
>   break;
> else if (c == L'/')
>   /* Replace UNIX dir separators with EFI standard ones */
>   *result++ = L'\\';
> else
>   *result++ = c;
> ---%<---
> 
> 
> (no need to resend)
> 
> 
>>                 *result++ = cmdline[i++];
>>         }
>>         *result = L'\0';
>> --
>> 2.27.0
>>

Sure, go ahead. I wasn't sure about the style, but yours looks more terse.

Thanks for queueing as a fix!