[PATCH v3] common: buffer access out-of-bounds

[PATCH v3] common: buffer access out-of-bounds

[Abdelrahman Youssef via B4 Relay]

From: Abdelrahman Youssef <abdelrahmanyossef12@gmail.com>

in file_detect_type() to detect file of type socfpga_xload you need at least
68 bytes bytes, so we need to check if we have enough bufsize.
So I moved it after checking if `bufsize >= 256`.

Signed-off-by: Abdelrahman Youssef <abdelrahmanyossef12@gmail.com>
---
This patch is a replacement of the last one because there were some issues with it
---
 common/filetype.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/common/filetype.c b/common/filetype.c
index 3690d4ae07..3f74871d7f 100644
--- a/common/filetype.c
+++ b/common/filetype.c
@@ -374,9 +374,6 @@ enum filetype file_detect_type(const void *_buf, size_t bufsize)
 	if (le32_to_cpu(buf[5]) == 0x504d5453)
 		return filetype_mxs_bootstream;
 
-	if (buf[16] == 0x31305341)
-		return filetype_socfpga_xload;
-
 	if (is_barebox_arm_head(_buf))
 		return filetype_arm_barebox;
 	if (buf[9] == 0x016f2818 || buf[9] == 0x18286f01)
@@ -388,7 +385,10 @@ enum filetype file_detect_type(const void *_buf, size_t bufsize)
 	if (bufsize < 256)
 		return filetype_unknown;
 
-	if (strncmp(buf8, "STM\x32", 4) == 0) {
+	if (buf[16] == 0x31305341)
+		return filetype_socfpga_xload;
+
+    if (strncmp(buf8, "STM\x32", 4) == 0) {
 		if (buf8[74] == 0x01) {
 			switch(le32_to_cpu(buf[63])) {
 			case 0x00000000:

---
base-commit: 9d47ff66c3892c5a6ddd4704993365a797fbeb68
change-id: 20241018-overflow-dc42def7e4f6

Best regards,
-- 
Abdelrahman Youssef <abdelrahmanyossef12@gmail.com>

Re: [PATCH v3] common: buffer access out-of-bounds

[Ahmad Fatoum]

Hello Abdelrahman,

Thanks for your patch!

On 18.10.24 17:26, Abdelrahman Youssef via B4 Relay wrote:
> From: Abdelrahman Youssef <abdelrahmanyossef12@gmail.com>
> 
> in file_detect_type() to detect file of type socfpga_xload you need at least
> 68 bytes bytes, so we need to check if we have enough bufsize.
> So I moved it after checking if `bufsize >= 256`.
> 
> Signed-off-by: Abdelrahman Youssef <abdelrahmanyossef12@gmail.com>
> ---
> This patch is a replacement of the last one because there were some issues with it

Please list the concrete changes done in the revision.

> ---
>  common/filetype.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/common/filetype.c b/common/filetype.c
> index 3690d4ae07..3f74871d7f 100644
> --- a/common/filetype.c
> +++ b/common/filetype.c
> @@ -374,9 +374,6 @@ enum filetype file_detect_type(const void *_buf, size_t bufsize)
>  	if (le32_to_cpu(buf[5]) == 0x504d5453)
>  		return filetype_mxs_bootstream;
>  
> -	if (buf[16] == 0x31305341)
> -		return filetype_socfpga_xload;
> -
>  	if (is_barebox_arm_head(_buf))
>  		return filetype_arm_barebox;
>  	if (buf[9] == 0x016f2818 || buf[9] == 0x18286f01)
> @@ -388,7 +385,10 @@ enum filetype file_detect_type(const void *_buf, size_t bufsize)
>  	if (bufsize < 256)
>  		return filetype_unknown;
>  
> -	if (strncmp(buf8, "STM\x32", 4) == 0) {
> +	if (buf[16] == 0x31305341)
> +		return filetype_socfpga_xload;
> +
> +    if (strncmp(buf8, "STM\x32", 4) == 0) {

This line should still not be in the diff. If you look closely, you'll
see that you replaced tabs with spaces. While this may sound overly
picky, it's quite important not to introduce random unrelated changes
into commits to make review easier and not needlessly complicate
use of git blame.

Cheers,
Ahmad

>  		if (buf8[74] == 0x01) {
>  			switch(le32_to_cpu(buf[63])) {
>  			case 0x00000000:
> 
> ---
> base-commit: 9d47ff66c3892c5a6ddd4704993365a797fbeb68
> change-id: 20241018-overflow-dc42def7e4f6
> 
> Best regards,


-- 
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 |