[PATCH mtd-utils] ubi-utils: FIX DIVISION BY ZERO in ubinize.c

[PATCH mtd-utils] ubi-utils: FIX DIVISION BY ZERO in ubinize.c

[Anton Moryakov]

Report of the static analyzer:
Variable vi->alignment, whose possible value set allows a zero value at ubinize.c:375, is used as a denominator at ubinize.c:410.

If you look at the code more closely, it will be clear that the vi->alignment parameter is obtained from an external file passed as a command line argument.

A check was also performed if you pass a test.ini file of the following type to the input:

[jffs2-volume]
mode=ubi
image=../jffs2.img
vol_id=1
vol_size=30MiB
vol_type=dynamic
vol_name=jffs2_volume
vol_flags=autoresize
vol_alignment=0

and execute the command:

./ubinize -o ubi.img -p 16KiB -m 512 -s 256 test.ini

we will get the result:

Floating point exception (core dumped)

Corrections explained:

Triggers found by static analyzer Svace.

Signed-off-by: Anton Moryakov <ant.v.moryakov@gmail.com>

---
 ubi-utils/ubinize.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/ubi-utils/ubinize.c b/ubi-utils/ubinize.c
index ac8c1e5..afffaa0 100644
--- a/ubi-utils/ubinize.c
+++ b/ubi-utils/ubinize.c
@@ -406,6 +406,14 @@ static int read_section(const struct ubigen_info *ui, const char *sname,
 		}
 	}
 
+
+	/* Validate alignment to avoid division by zero */
+	if (vi->alignment <= 0) {
+		fprintf(stderr, "Error: alignment value (%d) is invalid. It must be greater than 0.\n", vi->alignment);
+		vi->alignment = 1; /* Set a safe default value */
+		fprintf(stderr, "Warning: alignment set to default value (1).\n");
+	}
+
 	/* Initialize the rest of the volume information */
 	vi->data_pad = ui->leb_size % vi->alignment;
 	vi->usable_leb_size = ui->leb_size - vi->data_pad;
-- 
2.30.2


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH mtd-utils] ubi-utils: FIX DIVISION BY ZERO in ubinize.c

[Zhihao Cheng]

在 2024/12/17 22:00, Anton Moryakov 写道:
> Report of the static analyzer:
> Variable vi->alignment, whose possible value set allows a zero value at ubinize.c:375, is used as a denominator at ubinize.c:410.
> 
> If you look at the code more closely, it will be clear that the vi->alignment parameter is obtained from an external file passed as a command line argument.
> 
> A check was also performed if you pass a test.ini file of the following type to the input:
> 
> [jffs2-volume]
> mode=ubi
> image=../jffs2.img
> vol_id=1
> vol_size=30MiB
> vol_type=dynamic
> vol_name=jffs2_volume
> vol_flags=autoresize
> vol_alignment=0
> 
> and execute the command:
> 
> ./ubinize -o ubi.img -p 16KiB -m 512 -s 256 test.ini
> 
> we will get the result:
> 
> Floating point exception (core dumped)
> 
> Corrections explained:
> 
> Triggers found by static analyzer Svace.
> 
> Signed-off-by: Anton Moryakov <ant.v.moryakov@gmail.com>
> 
> ---
>   ubi-utils/ubinize.c | 8 ++++++++
>   1 file changed, 8 insertions(+)
> 
> diff --git a/ubi-utils/ubinize.c b/ubi-utils/ubinize.c
> index ac8c1e5..afffaa0 100644
> --- a/ubi-utils/ubinize.c
> +++ b/ubi-utils/ubinize.c
> @@ -406,6 +406,14 @@ static int read_section(const struct ubigen_info *ui, const char *sname,
>   		}
>   	}
>   
> +
> +	/* Validate alignment to avoid division by zero */
> +	if (vi->alignment <= 0) {
> +		fprintf(stderr, "Error: alignment value (%d) is invalid. It must be greater than 0.\n", vi->alignment);
> +		vi->alignment = 1; /* Set a safe default value */
> +		fprintf(stderr, "Warning: alignment set to default value (1).\n");
> +	}
> +

How about changing like this?
diff --git a/ubi-utils/ubinize.c b/ubi-utils/ubinize.c
index ac8c1e5..9c950b1 100644
--- a/ubi-utils/ubinize.c
+++ b/ubi-utils/ubinize.c
@@ -375,8 +375,8 @@ static int read_section(const struct ubigen_info 
*ui, const char *sname,
         vi->alignment = iniparser_getint(args.dict, buf, -1);
         if (vi->alignment == -1)
                 vi->alignment = 1;
-       else if (vi->id < 0)
-               return errmsg("negative volume alignment %d in section 
\"%s\"",
+       else if (vi->alignment <= 0)
+               return errmsg("not positive volume alignment %d in 
section \"%s\"",
                               vi->alignment, sname);

         verbose(args.verbose, "volume alignment: %d", vi->alignment);

I find that the 'vi->id' is misspelled as 'vi->alignment'.
>   	/* Initialize the rest of the volume information */
>   	vi->data_pad = ui->leb_size % vi->alignment;
>   	vi->usable_leb_size = ui->leb_size - vi->data_pad;
> 


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

[PATCH mtd-utils] ubi-utils: FIX DIVISION BY ZERO in ubinize.c

[Anton Moryakov]

Report of the static analyzer:
Variable vi->alignment, whose possible value set allows a zero value at ubinize.c:375, is used as a denominator at ubinize.c:410.

If you look at the code more closely, it will be clear that the vi->alignment parameter is obtained from an external file passed as a command line argument.

A check was also performed if you pass a test.ini file of the following type to the input:

[jffs2-volume]
mode=ubi
image=../jffs2.img
vol_id=1
vol_size=30MiB
vol_type=dynamic
vol_name=jffs2_volume
vol_flags=autoresize
vol_alignment=0

and execute the command:

./ubinize -o ubi.img -p 16KiB -m 512 -s 256 test.ini

we will get the result:

Floating point exception (core dumped)

Corrections explained:
Updated the validation logic for vi->alignment:
- Replaced the check for negative alignment (`vi->id < 0`) with a more comprehensive check for non-positive alignment (`vi->alignment <= 0`).
- Updated the corresponding error message to reflect the requirement for a positive volume alignment.
This ensures more robust validation and improves error clarity when invalid alignment values are encountered.

Triggers found by static analyzer Svace.

Signed-off-by: Anton Moryakov <ant.v.moryakov@gmail.com>

---
 ubi-utils/ubinize.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/ubi-utils/ubinize.c b/ubi-utils/ubinize.c
index ac8c1e5..9c950b1 100644
--- a/ubi-utils/ubinize.c
+++ b/ubi-utils/ubinize.c
@@ -375,8 +375,8 @@ static int read_section(const struct ubigen_info *ui, const char *sname,
 	vi->alignment = iniparser_getint(args.dict, buf, -1);
 	if (vi->alignment == -1)
 		vi->alignment = 1;
-	else if (vi->id < 0)
-		return errmsg("negative volume alignment %d in section \"%s\"",
+	else if (vi->alignment <= 0)
+		return errmsg("not positive volume alignment %d in section \"%s\"",
 			      vi->alignment, sname);
 
 	verbose(args.verbose, "volume alignment: %d", vi->alignment);
-- 
2.30.2


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH mtd-utils] ubi-utils: FIX DIVISION BY ZERO in ubinize.c

[Zhihao Cheng]

在 2024/12/25 3:40, Anton Moryakov 写道:
> Report of the static analyzer:
> Variable vi->alignment, whose possible value set allows a zero value at ubinize.c:375, is used as a denominator at ubinize.c:410.
> 
> If you look at the code more closely, it will be clear that the vi->alignment parameter is obtained from an external file passed as a command line argument.
> 
> A check was also performed if you pass a test.ini file of the following type to the input:
> 
> [jffs2-volume]
> mode=ubi
> image=../jffs2.img
> vol_id=1
> vol_size=30MiB
> vol_type=dynamic
> vol_name=jffs2_volume
> vol_flags=autoresize
> vol_alignment=0
> 
> and execute the command:
> 
> ./ubinize -o ubi.img -p 16KiB -m 512 -s 256 test.ini
> 
> we will get the result:
> 
> Floating point exception (core dumped)
> 
> Corrections explained:
> Updated the validation logic for vi->alignment:
> - Replaced the check for negative alignment (`vi->id < 0`) with a more comprehensive check for non-positive alignment (`vi->alignment <= 0`).
> - Updated the corresponding error message to reflect the requirement for a positive volume alignment.
> This ensures more robust validation and improves error clarity when invalid alignment values are encountered.
> 
> Triggers found by static analyzer Svace.
> 
> Signed-off-by: Anton Moryakov <ant.v.moryakov@gmail.com>
> 
> ---
>   ubi-utils/ubinize.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 

Reviewed-by: Zhihao Cheng <chengzhihao1@huawei.com>
> diff --git a/ubi-utils/ubinize.c b/ubi-utils/ubinize.c
> index ac8c1e5..9c950b1 100644
> --- a/ubi-utils/ubinize.c
> +++ b/ubi-utils/ubinize.c
> @@ -375,8 +375,8 @@ static int read_section(const struct ubigen_info *ui, const char *sname,
>   	vi->alignment = iniparser_getint(args.dict, buf, -1);
>   	if (vi->alignment == -1)
>   		vi->alignment = 1;
> -	else if (vi->id < 0)
> -		return errmsg("negative volume alignment %d in section \"%s\"",
> +	else if (vi->alignment <= 0)
> +		return errmsg("not positive volume alignment %d in section \"%s\"",
>   			      vi->alignment, sname);
>   
>   	verbose(args.verbose, "volume alignment: %d", vi->alignment);
> 


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/