[PATCH] scripts/common: fix write_file when opened with mmap

[PATCH] scripts/common: fix write_file when opened with mmap

[Andrej Picej]

Usage of bareboximd with -c option (Create checksum for FILE and write
it to the crc32 tag) was broken. Possibly by applying 2154de1cf36c
(bareboximd: Use mmap when possibly). The script fails with:

    $ ./scripts/bareboximd -c images/<barebox-image.img>
    Cannot write to images/<barebox-mage.img>: Bad address
    CRC: write crc token to images/<barebox-image.img> failed: -14
    Bad address

This has to do with the usage of "mmap" and "open" with O_TRUNC flag
which truncates the file length to 0. Writing to files fails with:
EFAULT (14) buf is outside your accessible address space.
Remove the truncate flag and truncate manually after writing the data.

This fixes the bareboximd script, which is now again usable with -c
option.

Signed-off-by: Anze Lesnik <anze.lesnik@norik.com>
Signed-off-by: Andrej Picej <andrej.picej@norik.com>
---
 scripts/common.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/scripts/common.c b/scripts/common.c
index 3f8dcd2d38..331988d593 100644
--- a/scripts/common.c
+++ b/scripts/common.c
@@ -107,7 +107,7 @@ int write_file(const char *filename, const void *buf, size_t size)
 	int fd, ret = 0;
 	int now;
 
-	fd = open(filename, O_WRONLY | O_TRUNC | O_CREAT,
+	fd = open(filename, O_WRONLY | O_CREAT,
 		  S_IRUSR | S_IWUSR | S_IRGRP | S_IROTH);
 	if (fd < 0) {
 		fprintf(stderr, "Cannot open %s: %s\n", filename, strerror(errno));
@@ -126,6 +126,11 @@ int write_file(const char *filename, const void *buf, size_t size)
 		buf += now;
 	}
 
+	if (ftruncate(fd, now) < 0) {
+		fprintf(stderr, "Cannot truncate file: %s", strerror(errno));
+		ret = -errno;
+	}
+
 out:
 	close(fd);
 
-- 
2.25.1


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH] scripts/common: fix write_file when opened with mmap

[Sascha Hauer]

Hi Andrej,

On Fri, Jan 28, 2022 at 09:00:33AM +0100, Andrej Picej wrote:
> Usage of bareboximd with -c option (Create checksum for FILE and write
> it to the crc32 tag) was broken. Possibly by applying 2154de1cf36c
> (bareboximd: Use mmap when possibly). The script fails with:
> 
>     $ ./scripts/bareboximd -c images/<barebox-image.img>
>     Cannot write to images/<barebox-mage.img>: Bad address
>     CRC: write crc token to images/<barebox-image.img> failed: -14
>     Bad address
> 
> This has to do with the usage of "mmap" and "open" with O_TRUNC flag
> which truncates the file length to 0. Writing to files fails with:
> EFAULT (14) buf is outside your accessible address space.
> Remove the truncate flag and truncate manually after writing the data.
> 
> This fixes the bareboximd script, which is now again usable with -c
> option.

That's a very unfortunate bug :(

> 
> Signed-off-by: Anze Lesnik <anze.lesnik@norik.com>
> Signed-off-by: Andrej Picej <andrej.picej@norik.com>
> ---
>  scripts/common.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/scripts/common.c b/scripts/common.c
> index 3f8dcd2d38..331988d593 100644
> --- a/scripts/common.c
> +++ b/scripts/common.c
> @@ -107,7 +107,7 @@ int write_file(const char *filename, const void *buf, size_t size)
>  	int fd, ret = 0;
>  	int now;
>  
> -	fd = open(filename, O_WRONLY | O_TRUNC | O_CREAT,

I added a comment above this while applying to prevent people from
"optimizing" this patch away.

> +	fd = open(filename, O_WRONLY | O_CREAT,
>  		  S_IRUSR | S_IWUSR | S_IRGRP | S_IROTH);
>  	if (fd < 0) {
>  		fprintf(stderr, "Cannot open %s: %s\n", filename, strerror(errno));
> @@ -126,6 +126,11 @@ int write_file(const char *filename, const void *buf, size_t size)
>  		buf += now;
>  	}
>  
> +	if (ftruncate(fd, now) < 0) {
> +		fprintf(stderr, "Cannot truncate file: %s", strerror(errno));
> +		ret = -errno;
> +	}

truncating to 'now' is only correct when we were able to write all bytes
at once. Otherwise we must truncate to the input size. I fixed this up
to the patch below.

Sascha

-----------------------------8<----------------------------

>From 738601e1258c55953284ee10801b26b9977918c2 Mon Sep 17 00:00:00 2001
From: Andrej Picej <andrej.picej@norik.com>
Date: Fri, 28 Jan 2022 09:00:33 +0100
Subject: [PATCH] scripts/common: fix write_file when opened with mmap

Usage of bareboximd with -c option (Create checksum for FILE and write
it to the crc32 tag) was broken. Possibly by applying 2154de1cf36c
(bareboximd: Use mmap when possibly). The script fails with:

    $ ./scripts/bareboximd -c images/<barebox-image.img>
    Cannot write to images/<barebox-mage.img>: Bad address
    CRC: write crc token to images/<barebox-image.img> failed: -14
    Bad address

This has to do with the usage of "mmap" and "open" with O_TRUNC flag
which truncates the file length to 0. Writing to files fails with:
EFAULT (14) buf is outside your accessible address space.
Remove the truncate flag and truncate manually after writing the data.

This fixes the bareboximd script, which is now again usable with -c
option.

Signed-off-by: Anze Lesnik <anze.lesnik@norik.com>
Signed-off-by: Andrej Picej <andrej.picej@norik.com>
Link: https://lore.barebox.org/20220128080033.167251-1-andrej.picej@norik.com
Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 scripts/common.c | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/scripts/common.c b/scripts/common.c
index b780b09941..154d6dffcb 100644
--- a/scripts/common.c
+++ b/scripts/common.c
@@ -105,26 +105,33 @@ int write_file(const char *filename, const void *buf, size_t size)
 {
 	int fd, ret = 0;
 	int now;
+	size_t left = size;
 
-	fd = open(filename, O_WRONLY | O_TRUNC | O_CREAT,
+	/* The same file may be mmapped currently, so can't use O_TRUNC here */
+	fd = open(filename, O_WRONLY | O_CREAT,
 		  S_IRUSR | S_IWUSR | S_IRGRP | S_IROTH);
 	if (fd < 0) {
 		fprintf(stderr, "Cannot open %s: %s\n", filename, strerror(errno));
 		return -errno;
 	}
 
-	while (size) {
-		now = write(fd, buf, size);
+	while (left) {
+		now = write(fd, buf, left);
 		if (now < 0) {
 			fprintf(stderr, "Cannot write to %s: %s\n", filename,
 				strerror(errno));
 			ret = -errno;
 			goto out;
 		}
-		size -= now;
+		left -= now;
 		buf += now;
 	}
 
+	if (ftruncate(fd, size) < 0) {
+		fprintf(stderr, "Cannot truncate file: %s", strerror(errno));
+		ret = -errno;
+	}
+
 out:
 	close(fd);
 
-- 
2.30.2

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH] scripts/common: fix write_file when opened with mmap

[Andrej Picej]

Hi Sascha,

On 31. 01. 22 10:23, Sascha Hauer wrote:
> Hi Andrej,
> 
> On Fri, Jan 28, 2022 at 09:00:33AM +0100, Andrej Picej wrote:
>> Usage of bareboximd with -c option (Create checksum for FILE and write
>> it to the crc32 tag) was broken. Possibly by applying 2154de1cf36c
>> (bareboximd: Use mmap when possibly). The script fails with:
>>
>>      $ ./scripts/bareboximd -c images/<barebox-image.img>
>>      Cannot write to images/<barebox-mage.img>: Bad address
>>      CRC: write crc token to images/<barebox-image.img> failed: -14
>>      Bad address
>>
>> This has to do with the usage of "mmap" and "open" with O_TRUNC flag
>> which truncates the file length to 0. Writing to files fails with:
>> EFAULT (14) buf is outside your accessible address space.
>> Remove the truncate flag and truncate manually after writing the data.
>>
>> This fixes the bareboximd script, which is now again usable with -c
>> option.
> 
> That's a very unfortunate bug :(
> 
>>
>> Signed-off-by: Anze Lesnik <anze.lesnik@norik.com>
>> Signed-off-by: Andrej Picej <andrej.picej@norik.com>
>> ---
>>   scripts/common.c | 7 ++++++-
>>   1 file changed, 6 insertions(+), 1 deletion(-)
>>
>> diff --git a/scripts/common.c b/scripts/common.c
>> index 3f8dcd2d38..331988d593 100644
>> --- a/scripts/common.c
>> +++ b/scripts/common.c
>> @@ -107,7 +107,7 @@ int write_file(const char *filename, const void *buf, size_t size)
>>   	int fd, ret = 0;
>>   	int now;
>>   
>> -	fd = open(filename, O_WRONLY | O_TRUNC | O_CREAT,
> 
> I added a comment above this while applying to prevent people from
> "optimizing" this patch away.
> 
>> +	fd = open(filename, O_WRONLY | O_CREAT,
>>   		  S_IRUSR | S_IWUSR | S_IRGRP | S_IROTH);
>>   	if (fd < 0) {
>>   		fprintf(stderr, "Cannot open %s: %s\n", filename, strerror(errno));
>> @@ -126,6 +126,11 @@ int write_file(const char *filename, const void *buf, size_t size)
>>   		buf += now;
>>   	}
>>   
>> +	if (ftruncate(fd, now) < 0) {
>> +		fprintf(stderr, "Cannot truncate file: %s", strerror(errno));
>> +		ret = -errno;
>> +	}
> 
> truncating to 'now' is only correct when we were able to write all bytes
> at once. Otherwise we must truncate to the input size. I fixed this up
> to the patch below.

That makes sense. Good thing you caught that.

Thanks.

> 
> Sascha
> 
> -----------------------------8<----------------------------
> 
>  From 738601e1258c55953284ee10801b26b9977918c2 Mon Sep 17 00:00:00 2001
> From: Andrej Picej <andrej.picej@norik.com>
> Date: Fri, 28 Jan 2022 09:00:33 +0100
> Subject: [PATCH] scripts/common: fix write_file when opened with mmap
> 
> Usage of bareboximd with -c option (Create checksum for FILE and write
> it to the crc32 tag) was broken. Possibly by applying 2154de1cf36c
> (bareboximd: Use mmap when possibly). The script fails with:
> 
>      $ ./scripts/bareboximd -c images/<barebox-image.img>
>      Cannot write to images/<barebox-mage.img>: Bad address
>      CRC: write crc token to images/<barebox-image.img> failed: -14
>      Bad address
> 
> This has to do with the usage of "mmap" and "open" with O_TRUNC flag
> which truncates the file length to 0. Writing to files fails with:
> EFAULT (14) buf is outside your accessible address space.
> Remove the truncate flag and truncate manually after writing the data.
> 
> This fixes the bareboximd script, which is now again usable with -c
> option.
> 
> Signed-off-by: Anze Lesnik <anze.lesnik@norik.com>
> Signed-off-by: Andrej Picej <andrej.picej@norik.com>
> Link: https://lore.barebox.org/20220128080033.167251-1-andrej.picej@norik.com
> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> ---
>   scripts/common.c | 15 +++++++++++----
>   1 file changed, 11 insertions(+), 4 deletions(-)
> 
> diff --git a/scripts/common.c b/scripts/common.c
> index b780b09941..154d6dffcb 100644
> --- a/scripts/common.c
> +++ b/scripts/common.c
> @@ -105,26 +105,33 @@ int write_file(const char *filename, const void *buf, size_t size)
>   {
>   	int fd, ret = 0;
>   	int now;
> +	size_t left = size;
>   
> -	fd = open(filename, O_WRONLY | O_TRUNC | O_CREAT,
> +	/* The same file may be mmapped currently, so can't use O_TRUNC here */
> +	fd = open(filename, O_WRONLY | O_CREAT,
>   		  S_IRUSR | S_IWUSR | S_IRGRP | S_IROTH);
>   	if (fd < 0) {
>   		fprintf(stderr, "Cannot open %s: %s\n", filename, strerror(errno));
>   		return -errno;
>   	}
>   
> -	while (size) {
> -		now = write(fd, buf, size);
> +	while (left) {
> +		now = write(fd, buf, left);
>   		if (now < 0) {
>   			fprintf(stderr, "Cannot write to %s: %s\n", filename,
>   				strerror(errno));
>   			ret = -errno;
>   			goto out;
>   		}
> -		size -= now;
> +		left -= now;
>   		buf += now;
>   	}
>   
> +	if (ftruncate(fd, size) < 0) {
> +		fprintf(stderr, "Cannot truncate file: %s", strerror(errno));
> +		ret = -errno;
> +	}
> +
>   out:
>   	close(fd);
>   
> 

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox