dfu: Fix data abort in dfu_free_entities()

PATCH New Sam Protsenko 2 participants 5 messages Jul 13, 2018 – Jul 24, 2018

Share & tools

Sam Protsenko

author

2018-07-13 13:35 UTC #0 lore raw

PATCH v2 1/3 dfu: Fix data abort in dfu_free_entities()

Commit 5d8fae79163e ("dfu: avoid memory leak") brings a regression which described below. This patch is effectively reverting that commit, adding corresponding comment to avoid such regressions in future.

In case of error in dfu_config_entities(), it frees "dfu" array, which leads to "data abort" in dfu_free_entities(), which tries to free the same array (and even tries to access it from linked list first). The issue occurs e.g. when partition table on device does not match $dfu_alt_info layout:

=> dfu 0 mmc 1
Couldn't find part #2 on mmc device #1
DFU entities configuration failed!
data abort

To fix this issue, do not free "dfu" array in dfu_config_entities(). It will be freed later in dfu_free_entities().

Tested on BeagleBone Black (where this regression was originally found).

Signed-off-by Sam Protsenko <[email protected]>

---
Changes in v2:
- Improve commit message by mentioning regression commit

 drivers/dfu/dfu.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/dfu/dfu.c +1 -1

index e7c91193b9..a3c09334b7 100644
@@ -462,7 +462,7 @@ int dfu_config_entities(char *env, char *interface, char *devstr)
 		ret = dfu_fill_entity(&dfu[i], s, alt_num_cnt, interface,
 				      devstr);
 		if (ret) {
-			free(dfu);
+			/* We will free "dfu" in dfu_free_entities() */
 			return -1;
 		}

Signature · 1 line

2.18.0

Sam Protsenko

author

2018-07-24 16:41 UTC #1 lore raw

PATCH v2 1/3 dfu: Fix data abort in dfu_free_entities()

Quoted 18 lines · Sam Protsenko

Commit 5d8fae79163e ("dfu: avoid memory leak") brings a regression which described below. This patch is effectively reverting that commit, adding corresponding comment to avoid such regressions in future.
In case of error in dfu_config_entities(), it frees "dfu" array, which leads to "data abort" in dfu_free_entities(), which tries to free the same array (and even tries to access it from linked list first). The issue occurs e.g. when partition table on device does not match $dfu_alt_info layout:
=> dfu 0 mmc 1
Couldn't find part #2 on mmc device #1
DFU entities configuration failed!
data abort
To fix this issue, do not free "dfu" array in dfu_config_entities(). It will be freed later in dfu_free_entities().

Quoted 7 lines

Tested on BeagleBone Black (where this regression was originally found).
Signed-off-by Sam Protsenko <[email protected]>
---
Changes in v2:
- Improve commit message by mentioning regression commit

Hi Lukasz,

Can you please review and merge this whole series?

Thanks!

Quoted 19 lines

 drivers/dfu/dfu.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/dfu/dfu.c +1 -2

index e7c91193b9..a3c09334b7 100644
@@ -462,7 +462,7 @@ int dfu_config_entities(char *env, char *interface, char *devstr)
                ret = dfu_fill_entity(&dfu[i], s, alt_num_cnt, interface,
                                      devstr);
                if (ret) {
-                       free(dfu);
+                       /* We will free "dfu" in dfu_free_entities() */
                        return -1;
                }
 
--

2.18.0

Lukasz Majewski

2018-07-24 21:14 UTC #2 lore raw

PATCH v2 1/3 dfu: Fix data abort in dfu_free_entities()

Hi Sam,

Quoted 31 lines

Quoted 20 lines · Sam Protsenko
Commit 5d8fae79163e ("dfu: avoid memory leak") brings a regression which described below. This patch is effectively reverting that commit, adding corresponding comment to avoid such regressions in future.
In case of error in dfu_config_entities(), it frees "dfu" array, which leads to "data abort" in dfu_free_entities(), which tries to free the same array (and even tries to access it from linked list first). The issue occurs e.g. when partition table on device does not match $dfu_alt_info layout:
=> dfu 0 mmc 1
Couldn't find part #2 on mmc device #1
DFU entities configuration failed!
data abort
To fix this issue, do not free "dfu" array in
dfu_config_entities(). It will be freed later in
dfu_free_entities().
Quoted 8 lines
Tested on BeagleBone Black (where this regression was originally found).
Signed-off-by Sam Protsenko <[email protected]>
---
Changes in v2:
- Improve commit message by mentioning regression commit

Quoted 3 lines

Hi Lukasz,
Can you please review and merge this whole series?

This has been already included:

http://git.denx.de/?p=u-boot/u-boot-dfu.git;a=shortlog;h=refs/heads/master

Marek has fetch this tree and will send PR soon.

Quoted 19 lines

Thanks!

Quoted 16 lines

 drivers/dfu/dfu.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/dfu/dfu.c +1 -1

index e7c91193b9..a3c09334b7 100644
@@ -462,7 +462,7 @@ int dfu_config_entities(char *env, char
 *interface, char *devstr) ret = dfu_fill_entity(&dfu[i], s,
 alt_num_cnt, interface, devstr);
                if (ret) {
-                       free(dfu);
+                       /* We will free "dfu" in
 dfu_free_entities() */ return -1;
                }

Quoted 3 lines

Quoted 3 lines
--
2.18.0

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/20180724/64197372/attachment.sig>

Sam Protsenko

author

2018-07-13 13:35 UTC #3 lore raw

PATCH v2 2/3 dfu: Fix memory leak in dfu_init_env_entities()

In reply to Sam Protsenko

In case of error in dfu_init_env_entities(), env_bkp will leak. Fix it by providing single return path.

Signed-off-by Sam Protsenko <[email protected]>

---
Changes in v2:
- None

 drivers/dfu/dfu.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

drivers/dfu/dfu.c +4 -3

index a3c09334b7..5b9abd685d 100644
@@ -56,7 +56,7 @@ int dfu_init_env_entities(char *interface, char *devstr)
 {
 	const char *str_env;
 	char *env_bkp;
-	int ret;
+	int ret = 0;
 
 #ifdef CONFIG_SET_DFU_ALT_INFO
 	set_dfu_alt_info(interface, devstr);

@@ -71,11 +71,12 @@ int dfu_init_env_entities(char *interface, char *devstr)
 	ret = dfu_config_entities(env_bkp, interface, devstr);
 	if (ret) {
 		pr_err("DFU entities configuration failed!\n");
-		return ret;
+		goto done;
 	}
 
+done:
 	free(env_bkp);
-	return 0;
+	return ret;
 }
 
 static unsigned char *dfu_buf;

Signature · 1 line

2.18.0

Sam Protsenko

author

2018-07-13 13:35 UTC #4 lore raw

PATCH v2 3/3 dfu: Provide more verbose error message

In reply to Sam Protsenko

It might be useful for user to see some human-readable root cause message in addition to "configuration failed" message, so that the issue can be fixed quickly.

Signed-off-by Sam Protsenko <[email protected]>

---
Changes in v2:
- None

 drivers/dfu/dfu.c | 1 +
 1 file changed, 1 insertion(+)

drivers/dfu/dfu.c +1 -0

index 5b9abd685d..318949529b 100644
@@ -71,6 +71,7 @@ int dfu_init_env_entities(char *interface, char *devstr)
 	ret = dfu_config_entities(env_bkp, interface, devstr);
 	if (ret) {
 		pr_err("DFU entities configuration failed!\n");
+		pr_err("(partition table does not match dfu_alt_info?)\n");
 		goto done;
 	}

Signature · 1 line

2.18.0