[U-Boot] [RFC 0/3] efi_loader: allow writing to FAT

[U-Boot] [RFC 0/3] efi_loader: allow writing to FAT

[Heinrich Schuchardt]

Running UEFI Self-Certifcation Test (SCT) will require to support
writing to a FAT file system.

Writing to the FAT root directory from EFI fails because the leading
slash of the file path is interpreted as part of the filename.

With the 1st patch we can write to the root directory.

A unit test for writing a new file to the FAT root directory is supplied.

I am still unhappy that we cannot write to subdirectories. I therefore
mark the series as RFC.

Heinrich Schuchardt (3):
  fs: fat: cannot write to subdirectories
  efi_selftest: imply FAT, FAT_WRITE
  efi_selftest: test writing to file

 fs/fat/fat_write.c                           | 18 ++++++-
 lib/efi_selftest/Kconfig                     |  2 +
 lib/efi_selftest/efi_selftest_block_device.c | 56 ++++++++++++++++++++
 3 files changed, 75 insertions(+), 1 deletion(-)

-- 
2.17.0

[U-Boot] [RFC 1/3] fs: fat: cannot write to subdirectories

[Heinrich Schuchardt]

fs_fat_write() is not able to write to subdirectories.

Currently if a filepath with a leading slash is passed, the slash is
treated as part of the filename to be created in the root directory.

Strip leading (back-)slashes.

Check that the remaining filename does not contain any illegal characters
(<>:"/\|?*). This way we will throw an error when trying to write to a
subdirectory.

Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
---
 fs/fat/fat_write.c | 18 +++++++++++++++++-
 1 file changed, 17 insertions(+), 1 deletion(-)

diff --git a/fs/fat/fat_write.c b/fs/fat/fat_write.c
index 5ca8fcda73c..f32745febb4 100644
--- a/fs/fat/fat_write.c
+++ b/fs/fat/fat_write.c
@@ -908,9 +908,11 @@ static int do_fat_write(const char *filename, void *buffer, loff_t size,
 	volume_info volinfo;
 	fsdata datablock;
 	fsdata *mydata = &datablock;
-	int cursect;
+	int cursect, i;
 	int ret = -1, name_len;
 	char l_filename[VFAT_MAXLEN_BYTES];
+	char bad[2] = " ";
+	const char illegal[] = "<>:\"/\\|?*";
 
 	*actwrite = size;
 	dir_curclust = 0;
@@ -970,6 +972,20 @@ static int do_fat_write(const char *filename, void *buffer, loff_t size,
 	}
 	dentptr = (dir_entry *) do_fat_read_at_block;
 
+	/* Strip leading (back-)slashes */
+	while (*filename == '/')
+		++filename;
+	while (*filename == '\\')
+		++filename;
+	/* Check that the filename is valid */
+	for (i = 0; i < strlen(illegal); ++i) {
+		*bad = illegal[i];
+		if (strstr(filename, bad)) {
+			printf("FAT: illegal filename (%s)\n", filename);
+			return -1;
+		}
+	}
+
 	name_len = strlen(filename);
 	if (name_len >= VFAT_MAXLEN_BYTES)
 		name_len = VFAT_MAXLEN_BYTES - 1;
-- 
2.17.0

[U-Boot] [RFC 1/3] fs: fat: cannot write to subdirectories

[Lukasz Majewski]

On Sat, 26 May 2018 10:34:45 +0200
Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:

> fs_fat_write() is not able to write to subdirectories.
> 
> Currently if a filepath with a leading slash is passed, the slash is
> treated as part of the filename to be created in the root directory.
> 
> Strip leading (back-)slashes.
> 
> Check that the remaining filename does not contain any illegal
> characters (<>:"/\|?*). This way we will throw an error when trying
> to write to a subdirectory.
> 
> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> ---
>  fs/fat/fat_write.c | 18 +++++++++++++++++-
>  1 file changed, 17 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/fat/fat_write.c b/fs/fat/fat_write.c
> index 5ca8fcda73c..f32745febb4 100644
> --- a/fs/fat/fat_write.c
> +++ b/fs/fat/fat_write.c
> @@ -908,9 +908,11 @@ static int do_fat_write(const char *filename,
> void *buffer, loff_t size, volume_info volinfo;
>  	fsdata datablock;
>  	fsdata *mydata = &datablock;
> -	int cursect;
> +	int cursect, i;
>  	int ret = -1, name_len;
>  	char l_filename[VFAT_MAXLEN_BYTES];
> +	char bad[2] = " ";
> +	const char illegal[] = "<>:\"/\\|?*";
>  
>  	*actwrite = size;
>  	dir_curclust = 0;
> @@ -970,6 +972,20 @@ static int do_fat_write(const char *filename,
> void *buffer, loff_t size, }
>  	dentptr = (dir_entry *) do_fat_read_at_block;
>  
> +	/* Strip leading (back-)slashes */
> +	while (*filename == '/')
> +		++filename;
> +	while (*filename == '\\')
> +		++filename;
> +	/* Check that the filename is valid */
> +	for (i = 0; i < strlen(illegal); ++i) {
> +		*bad = illegal[i];
> +		if (strstr(filename, bad)) {
> +			printf("FAT: illegal filename (%s)\n",
> filename);
> +			return -1;
> +		}
> +	}
> +
>  	name_len = strlen(filename);
>  	if (name_len >= VFAT_MAXLEN_BYTES)
>  		name_len = VFAT_MAXLEN_BYTES - 1;

Reviewed-by: Lukasz Majewski <lukma@denx.de>


Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 488 bytes
Desc: OpenPGP digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20180528/81898d40/attachment.sig>

[U-Boot] [RFC 2/3] efi_selftest: imply FAT, FAT_WRITE

[Heinrich Schuchardt]

efi_selftest_block_device accesses a FAT file system.

Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
---
 lib/efi_selftest/Kconfig | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/lib/efi_selftest/Kconfig b/lib/efi_selftest/Kconfig
index 3b5f3a1230b..59f9f36801c 100644
--- a/lib/efi_selftest/Kconfig
+++ b/lib/efi_selftest/Kconfig
@@ -1,6 +1,8 @@
 config CMD_BOOTEFI_SELFTEST
 	bool "Allow booting an EFI efi_selftest"
 	depends on CMD_BOOTEFI
+	imply FAT
+	imply FAT_WRITE
 	help
 	  This adds an EFI test application to U-Boot that can be executed
 	  with the 'bootefi selftest' command. It provides extended tests of
-- 
2.17.0

[U-Boot] [RFC 3/3] efi_selftest: test writing to file

[Heinrich Schuchardt]

Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
---
 lib/efi_selftest/efi_selftest_block_device.c | 56 ++++++++++++++++++++
 1 file changed, 56 insertions(+)

diff --git a/lib/efi_selftest/efi_selftest_block_device.c b/lib/efi_selftest/efi_selftest_block_device.c
index 4af8bd8cb28..9209ce678ee 100644
--- a/lib/efi_selftest/efi_selftest_block_device.c
+++ b/lib/efi_selftest/efi_selftest_block_device.c
@@ -377,6 +377,7 @@ static int execute(void)
 			"Wrong volume label '%ps', expected 'U-BOOT TEST'\n",
 			system_info.info.volume_label);
 	}
+	/* Read file */
 	ret = root->open(root, &file, (s16 *)L"hello.txt", EFI_FILE_MODE_READ,
 			 0);
 	if (ret != EFI_SUCCESS) {
@@ -389,6 +390,11 @@ static int execute(void)
 		efi_st_error("Failed to read file\n");
 		return EFI_ST_FAILURE;
 	}
+	if (buf_size != 13) {
+		efi_st_error("Wrong number of bytes read: %u\n",
+			     (unsigned int)buf_size);
+		return EFI_ST_FAILURE;
+	}
 	if (efi_st_memcmp(buf, "Hello world!", 12)) {
 		efi_st_error("Unexpected file content\n");
 		return EFI_ST_FAILURE;
@@ -399,6 +405,56 @@ static int execute(void)
 		return EFI_ST_FAILURE;
 	}
 	ret = root->close(root);
+	/* Write file */
+	ret = root->open(root, &file, (s16 *)L"u-boot.txt",
+			 EFI_FILE_MODE_WRITE | EFI_FILE_MODE_CREATE, 0);
+	if (ret != EFI_SUCCESS) {
+		efi_st_error("Failed to open file\n");
+		return EFI_ST_FAILURE;
+	}
+	buf_size = 7;
+	boottime->set_mem(buf, sizeof(buf), 0);
+	boottime->copy_mem(buf, "U-Boot", buf_size);
+	ret = file->write(file, &buf_size, buf);
+	if (ret != EFI_SUCCESS || buf_size != 7) {
+		efi_st_error("Failed to write file\n");
+		return EFI_ST_FAILURE;
+	}
+	ret = file->close(file);
+	if (ret != EFI_SUCCESS) {
+		efi_st_error("Failed to close file\n");
+		return EFI_ST_FAILURE;
+	}
+	ret = root->close(root);
+	/* Verify file */
+	boottime->set_mem(buf, sizeof(buf), 0);
+	ret = root->open(root, &file, (s16 *)L"u-boot.txt", EFI_FILE_MODE_READ,
+			 0);
+	if (ret != EFI_SUCCESS) {
+		efi_st_error("Failed to open file\n");
+		return EFI_ST_FAILURE;
+	}
+	buf_size = sizeof(buf) - 1;
+	ret = file->read(file, &buf_size, buf);
+	if (ret != EFI_SUCCESS) {
+		efi_st_error("Failed to read file\n");
+		return EFI_ST_FAILURE;
+	}
+	if (buf_size != 7) {
+		efi_st_error("Wrong number of bytes read: %u\n",
+			     (unsigned int) buf_size);
+		return EFI_ST_FAILURE;
+	}
+	if (efi_st_memcmp(buf, "U-Boot", 7)) {
+		efi_st_error("Unexpected file content %s\n", buf);
+		return EFI_ST_FAILURE;
+	}
+	ret = file->close(file);
+	if (ret != EFI_SUCCESS) {
+		efi_st_error("Failed to close file\n");
+		return EFI_ST_FAILURE;
+	}
+	ret = root->close(root);
 	if (ret != EFI_SUCCESS) {
 		efi_st_error("Failed to close volume\n");
 		return EFI_ST_FAILURE;
-- 
2.17.0

[U-Boot] [RFC 3/3] efi_selftest: test writing to file

[AKASHI, Takahiro]

Heinrich,

On Sat, May 26, 2018 at 10:34:47AM +0200, Heinrich Schuchardt wrote:
> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> ---
>  lib/efi_selftest/efi_selftest_block_device.c | 56 ++++++++++++++++++++
>  1 file changed, 56 insertions(+)
> 
> diff --git a/lib/efi_selftest/efi_selftest_block_device.c b/lib/efi_selftest/efi_selftest_block_device.c
> index 4af8bd8cb28..9209ce678ee 100644
> --- a/lib/efi_selftest/efi_selftest_block_device.c
> +++ b/lib/efi_selftest/efi_selftest_block_device.c
> @@ -377,6 +377,7 @@ static int execute(void)
>  			"Wrong volume label '%ps', expected 'U-BOOT TEST'\n",
>  			system_info.info.volume_label);
>  	}
> +	/* Read file */
>  	ret = root->open(root, &file, (s16 *)L"hello.txt", EFI_FILE_MODE_READ,
>  			 0);
>  	if (ret != EFI_SUCCESS) {
> @@ -389,6 +390,11 @@ static int execute(void)
>  		efi_st_error("Failed to read file\n");
>  		return EFI_ST_FAILURE;
>  	}
> +	if (buf_size != 13) {
> +		efi_st_error("Wrong number of bytes read: %u\n",
> +			     (unsigned int)buf_size);
> +		return EFI_ST_FAILURE;
> +	}
>  	if (efi_st_memcmp(buf, "Hello world!", 12)) {
>  		efi_st_error("Unexpected file content\n");
>  		return EFI_ST_FAILURE;
> @@ -399,6 +405,56 @@ static int execute(void)
>  		return EFI_ST_FAILURE;
>  	}
>  	ret = root->close(root);

We should not close 'root' here as
> +	/* Write file */
> +	ret = root->open(root, &file, (s16 *)L"u-boot.txt",
> +			 EFI_FILE_MODE_WRITE | EFI_FILE_MODE_CREATE, 0);

we will use it here.

> +	if (ret != EFI_SUCCESS) {
> +		efi_st_error("Failed to open file\n");
> +		return EFI_ST_FAILURE;
> +	}
> +	buf_size = 7;
> +	boottime->set_mem(buf, sizeof(buf), 0);
> +	boottime->copy_mem(buf, "U-Boot", buf_size);
> +	ret = file->write(file, &buf_size, buf);
> +	if (ret != EFI_SUCCESS || buf_size != 7) {
> +		efi_st_error("Failed to write file\n");
> +		return EFI_ST_FAILURE;
> +	}
> +	ret = file->close(file);
> +	if (ret != EFI_SUCCESS) {
> +		efi_st_error("Failed to close file\n");
> +		return EFI_ST_FAILURE;
> +	}
> +	ret = root->close(root);

ditto

Thanks,
-Takahiro AKASHI

> +	/* Verify file */
> +	boottime->set_mem(buf, sizeof(buf), 0);
> +	ret = root->open(root, &file, (s16 *)L"u-boot.txt", EFI_FILE_MODE_READ,
> +			 0);
> +	if (ret != EFI_SUCCESS) {
> +		efi_st_error("Failed to open file\n");
> +		return EFI_ST_FAILURE;
> +	}
> +	buf_size = sizeof(buf) - 1;
> +	ret = file->read(file, &buf_size, buf);
> +	if (ret != EFI_SUCCESS) {
> +		efi_st_error("Failed to read file\n");
> +		return EFI_ST_FAILURE;
> +	}
> +	if (buf_size != 7) {
> +		efi_st_error("Wrong number of bytes read: %u\n",
> +			     (unsigned int) buf_size);
> +		return EFI_ST_FAILURE;
> +	}
> +	if (efi_st_memcmp(buf, "U-Boot", 7)) {
> +		efi_st_error("Unexpected file content %s\n", buf);
> +		return EFI_ST_FAILURE;
> +	}
> +	ret = file->close(file);
> +	if (ret != EFI_SUCCESS) {
> +		efi_st_error("Failed to close file\n");
> +		return EFI_ST_FAILURE;
> +	}
> +	ret = root->close(root);
>  	if (ret != EFI_SUCCESS) {
>  		efi_st_error("Failed to close volume\n");
>  		return EFI_ST_FAILURE;
> -- 
> 2.17.0
> 

[U-Boot] [RFC 3/3] efi_selftest: test writing to file

[Heinrich Schuchardt]

On 06/01/2018 09:13 AM, AKASHI, Takahiro wrote:
> Heinrich,
> 
> On Sat, May 26, 2018 at 10:34:47AM +0200, Heinrich Schuchardt wrote:
>> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
>> ---
>>  lib/efi_selftest/efi_selftest_block_device.c | 56 ++++++++++++++++++++
>>  1 file changed, 56 insertions(+)
>>
>> diff --git a/lib/efi_selftest/efi_selftest_block_device.c b/lib/efi_selftest/efi_selftest_block_device.c
>> index 4af8bd8cb28..9209ce678ee 100644
>> --- a/lib/efi_selftest/efi_selftest_block_device.c
>> +++ b/lib/efi_selftest/efi_selftest_block_device.c
>> @@ -377,6 +377,7 @@ static int execute(void)
>>  			"Wrong volume label '%ps', expected 'U-BOOT TEST'\n",
>>  			system_info.info.volume_label);
>>  	}
>> +	/* Read file */
>>  	ret = root->open(root, &file, (s16 *)L"hello.txt", EFI_FILE_MODE_READ,
>>  			 0);
>>  	if (ret != EFI_SUCCESS) {
>> @@ -389,6 +390,11 @@ static int execute(void)
>>  		efi_st_error("Failed to read file\n");
>>  		return EFI_ST_FAILURE;
>>  	}
>> +	if (buf_size != 13) {
>> +		efi_st_error("Wrong number of bytes read: %u\n",
>> +			     (unsigned int)buf_size);
>> +		return EFI_ST_FAILURE;
>> +	}
>>  	if (efi_st_memcmp(buf, "Hello world!", 12)) {
>>  		efi_st_error("Unexpected file content\n");
>>  		return EFI_ST_FAILURE;
>> @@ -399,6 +405,56 @@ static int execute(void)
>>  		return EFI_ST_FAILURE;
>>  	}
>>  	ret = root->close(root);
> 

Your observation is correct. This is why I already sent a revised
version of the patch series. I unfortunately missed to put you on copy.

You can find the current patch here:

[PATCH v2 3/3] efi_selftest: test writing to file
https://patchwork.ozlabs.org/patch/921166/
https://lists.denx.de/pipermail/u-boot/2018-May/329730.html

Best regards

Heinrich

> We should not close 'root' here as
>> +	/* Write file */
>> +	ret = root->open(root, &file, (s16 *)L"u-boot.txt",
>> +			 EFI_FILE_MODE_WRITE | EFI_FILE_MODE_CREATE, 0);
> 
> we will use it here.
> 
>> +	if (ret != EFI_SUCCESS) {
>> +		efi_st_error("Failed to open file\n");
>> +		return EFI_ST_FAILURE;
>> +	}
>> +	buf_size = 7;
>> +	boottime->set_mem(buf, sizeof(buf), 0);
>> +	boottime->copy_mem(buf, "U-Boot", buf_size);
>> +	ret = file->write(file, &buf_size, buf);
>> +	if (ret != EFI_SUCCESS || buf_size != 7) {
>> +		efi_st_error("Failed to write file\n");
>> +		return EFI_ST_FAILURE;
>> +	}
>> +	ret = file->close(file);
>> +	if (ret != EFI_SUCCESS) {
>> +		efi_st_error("Failed to close file\n");
>> +		return EFI_ST_FAILURE;
>> +	}
>> +	ret = root->close(root);
> 
> ditto
> 
> Thanks,
> -Takahiro AKASHI
> 
>> +	/* Verify file */
>> +	boottime->set_mem(buf, sizeof(buf), 0);
>> +	ret = root->open(root, &file, (s16 *)L"u-boot.txt", EFI_FILE_MODE_READ,
>> +			 0);
>> +	if (ret != EFI_SUCCESS) {
>> +		efi_st_error("Failed to open file\n");
>> +		return EFI_ST_FAILURE;
>> +	}
>> +	buf_size = sizeof(buf) - 1;
>> +	ret = file->read(file, &buf_size, buf);
>> +	if (ret != EFI_SUCCESS) {
>> +		efi_st_error("Failed to read file\n");
>> +		return EFI_ST_FAILURE;
>> +	}
>> +	if (buf_size != 7) {
>> +		efi_st_error("Wrong number of bytes read: %u\n",
>> +			     (unsigned int) buf_size);
>> +		return EFI_ST_FAILURE;
>> +	}
>> +	if (efi_st_memcmp(buf, "U-Boot", 7)) {
>> +		efi_st_error("Unexpected file content %s\n", buf);
>> +		return EFI_ST_FAILURE;
>> +	}
>> +	ret = file->close(file);
>> +	if (ret != EFI_SUCCESS) {
>> +		efi_st_error("Failed to close file\n");
>> +		return EFI_ST_FAILURE;
>> +	}
>> +	ret = root->close(root);
>>  	if (ret != EFI_SUCCESS) {
>>  		efi_st_error("Failed to close volume\n");
>>  		return EFI_ST_FAILURE;
>> -- 
>> 2.17.0
>>
>