dfu: mmc: check if mmc device exists in mmc_block_op()

PATCH Superseded Przemyslaw Marczak 3 participants 13 messages Dec 11, 2014 – Dec 16, 2014 USB

Share & tools

Przemyslaw Marczak

author

2014-12-11 11:02 UTC #0 lore raw

PATCH 1/2 dfu: mmc: check if mmc device exists in mmc_block_op()

The function mmc_block_op() is the last function before the physicall data write, but the mmc device pointer is not checked. If mmc device not exists, then data abort will occur. To avoid this, first the mmc device pointer is checked.

Signed-off-by Przemyslaw Marczak <[email protected]>

---
 drivers/dfu/dfu_mmc.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

drivers/dfu/dfu_mmc.c +7 -1

index 72fa03e..41ac188 100644
@@ -40,10 +40,16 @@ static int mmc_access_part(struct dfu_entity *dfu, struct mmc *mmc, int part)
 static int mmc_block_op(enum dfu_op op, struct dfu_entity *dfu,
 			u64 offset, void *buf, long *len)
 {
-	struct mmc *mmc = find_mmc_device(dfu->data.mmc.dev_num);
+	struct mmc *mmc;
 	u32 blk_start, blk_count, n = 0;
 	int ret, part_num_bkp = 0;
 
+	mmc = find_mmc_device(dfu->data.mmc.dev_num);
+	if (!mmc) {
+		printf("Device MMC %d - not found!", dfu->data.mmc.dev_num);
+		return -ENODEV;
+	}
+
 	/*
 	 * We must ensure that we work in lba_blk_size chunks, so ALIGN
 	 * this value.

Signature · 1 line

1.9.1

Przemyslaw Marczak

author

2014-12-11 11:03 UTC #1 lore raw

PATCH 2/2 gadget: f_thor: check pointers before use in download_tail()

Some pointers in function download_tail() were not checked before the use. This could possibly cause the data abort. To avoid this, check if the pointers are not null is added.

Signed-off-by Przemyslaw Marczak <[email protected]>

---
 drivers/usb/gadget/f_thor.c | 16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)

drivers/usb/gadget/f_thor.c +14 -2

index 78519fa..9f77ba6 100644
@@ -205,12 +205,24 @@ static long long int download_head(unsigned long long total,
 
 static int download_tail(long long int left, int cnt)
 {
-	struct dfu_entity *dfu_entity = dfu_get_entity(alt_setting_num);
-	void *transfer_buffer = dfu_get_buf(dfu_entity);
+	struct dfu_entity *dfu_entity;
+	void *transfer_buffer;
 	int ret;
 
 	debug("%s: left: %llu cnt: %d\n", __func__, left, cnt);
 
+	dfu_entity = dfu_get_entity(alt_setting_num);
+	if (!dfu_entity) {
+		printf("Alt setting: %d entity not found!\n", alt_setting_num);
+		return -ENOENT;
+	}
+
+	transfer_buffer = dfu_get_buf(dfu_entity);
+	if (!transfer_buffer) {
+		error("Transfer buffer not allocated!");
+		return -ENXIO;
+	}
+
 	if (left) {
 		ret = dfu_write(dfu_entity, transfer_buffer, left, cnt++);
 		if (ret) {

Signature · 1 line

1.9.1

Lukasz Majewski

2014-12-12 08:38 UTC #2 lore raw

PATCH 2/2 gadget: f_thor: check pointers before use in download_tail()

Hi Przemyslaw,

Quoted 20 lines

Some pointers in function download_tail() were not checked before the use. This could possibly cause the data abort. To avoid this, check if the pointers are not null is added.
Signed-off-by Przemyslaw Marczak <[email protected]>
---
 drivers/usb/gadget/f_thor.c | 16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)
drivers/usb/gadget/f_thor.c +0 -2
index 78519fa..9f77ba6 100644
@@ -205,12 +205,24 @@ static long long int download_head(unsigned
 long long total, 
 static int download_tail(long long int left, int cnt)
 {
-	struct dfu_entity *dfu_entity =
 dfu_get_entity(alt_setting_num);
-	void *transfer_buffer = dfu_get_buf(dfu_entity);

drivers/usb/gadget/f_thor.c static long long int download_head(unsigned · line 218

Quoted 10 lines

drivers/usb/gadget/f_thor.c +5 -0

+	struct dfu_entity *dfu_entity;
+	void *transfer_buffer;
 	int ret;
 
 	debug("%s: left: %llu cnt: %d\n", __func__, left, cnt);
 
+	dfu_entity = dfu_get_entity(alt_setting_num);
+	if (!dfu_entity) {
+		printf("Alt setting: %d entity not found!\n",
 alt_setting_num);

	Please change printf() -> error()

Quoted 12 lines

drivers/usb/gadget/f_thor.c +9 -0

+		return -ENOENT;
+	}
+
+	transfer_buffer = dfu_get_buf(dfu_entity);
+	if (!transfer_buffer) {
+		error("Transfer buffer not allocated!");
+		return -ENXIO;
+	}
+
 	if (left) {

ret = dfu_write(dfu_entity, transfer_buffer, left,
cnt++); if (ret) {

Signature · 5 lines

Best regards,

Lukasz Majewski

Samsung R&D Institute Poland (SRPOL) | Linux Platform Group

Lukasz Majewski

2014-12-12 08:38 UTC #3 lore raw

PATCH 1/2 dfu: mmc: check if mmc device exists in mmc_block_op()

In reply to Przemyslaw Marczak

Hi Przemyslaw,

Quoted 17 lines

The function mmc_block_op() is the last function before the physicall data write, but the mmc device pointer is not checked. If mmc device not exists, then data abort will occur. To avoid this, first the mmc device pointer is checked.
Signed-off-by Przemyslaw Marczak <[email protected]>
---
 drivers/dfu/dfu_mmc.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)
drivers/dfu/dfu_mmc.c +0 -0
index 72fa03e..41ac188 100644
@@ -40,10 +40,16 @@ static int mmc_access_part(struct dfu_entity
 *dfu, struct mmc *mmc, int part) static int mmc_block_op(enum dfu_op
 op, struct dfu_entity *dfu, u64 offset, void *buf, long *len)

drivers/dfu/dfu_mmc.c static int mmc_access_part(struct dfu_entity · line 50

Quoted 10 lines

drivers/dfu/dfu_mmc.c +4 -1

 {
-	struct mmc *mmc = find_mmc_device(dfu->data.mmc.dev_num);
+	struct mmc *mmc;
 	u32 blk_start, blk_count, n = 0;
 	int ret, part_num_bkp = 0;
 
+	mmc = find_mmc_device(dfu->data.mmc.dev_num);
+	if (!mmc) {
+		printf("Device MMC %d - not found!",
 dfu->data.mmc.dev_num);

Please change printf() -> error();

Quoted 7 lines

drivers/dfu/dfu_mmc.c +3 -0

+		return -ENODEV;
+	}
+
 	/*
 	 * We must ensure that we work in lba_blk_size chunks, so

ALIGN
* this value.

Signature · 5 lines

Best regards,

Lukasz Majewski

Samsung R&D Institute Poland (SRPOL) | Linux Platform Group

Przemyslaw Marczak

author

2014-12-15 09:34 UTC #4 lore raw

PATCH v2 1/3 dfu: mmc: check if mmc device exists in mmc_block_op()

In reply to Przemyslaw Marczak

Signed-off-by Przemyslaw Marczak <[email protected]>

---
Change v2:
- mmc_block_op(): change printf() to error()

---
 drivers/dfu/dfu_mmc.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

drivers/dfu/dfu_mmc.c +7 -1

index 72fa03e..62d72fe 100644
@@ -40,10 +40,16 @@ static int mmc_access_part(struct dfu_entity *dfu, struct mmc *mmc, int part)
 static int mmc_block_op(enum dfu_op op, struct dfu_entity *dfu,
 			u64 offset, void *buf, long *len)
 {
-	struct mmc *mmc = find_mmc_device(dfu->data.mmc.dev_num);
+	struct mmc *mmc;
 	u32 blk_start, blk_count, n = 0;
 	int ret, part_num_bkp = 0;
 
+	mmc = find_mmc_device(dfu->data.mmc.dev_num);
+	if (!mmc) {
+		error("Device MMC %d - not found!", dfu->data.mmc.dev_num);
+		return -ENODEV;
+	}
+
 	/*
 	 * We must ensure that we work in lba_blk_size chunks, so ALIGN
 	 * this value.

Signature · 1 line

1.9.1

Przemyslaw Marczak

author

2014-12-15 09:34 UTC #5 lore raw

PATCH v2 2/3 gadget: f_thor: check pointers before use in download_tail()

Some pointers in function download_tail() were not checked before the use. This could possibly cause the data abort. To avoid this, check if the pointers are not null is added.

Signed-off-by Przemyslaw Marczak <[email protected]>

---
Change v2:
- download_tail(): change printf() to error()

---
 drivers/usb/gadget/f_thor.c | 16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)

drivers/usb/gadget/f_thor.c +14 -2

index 78519fa..2d0410d 100644
@@ -205,12 +205,24 @@ static long long int download_head(unsigned long long total,
 
 static int download_tail(long long int left, int cnt)
 {
-	struct dfu_entity *dfu_entity = dfu_get_entity(alt_setting_num);
-	void *transfer_buffer = dfu_get_buf(dfu_entity);
+	struct dfu_entity *dfu_entity;
+	void *transfer_buffer;
 	int ret;
 
 	debug("%s: left: %llu cnt: %d\n", __func__, left, cnt);
 
+	dfu_entity = dfu_get_entity(alt_setting_num);
+	if (!dfu_entity) {
+		error("Alt setting: %d entity not found!\n", alt_setting_num);
+		return -ENOENT;
+	}
+
+	transfer_buffer = dfu_get_buf(dfu_entity);
+	if (!transfer_buffer) {
+		error("Transfer buffer not allocated!");
+		return -ENXIO;
+	}
+
 	if (left) {
 		ret = dfu_write(dfu_entity, transfer_buffer, left, cnt++);
 		if (ret) {

Signature · 1 line

1.9.1

Lukasz Majewski

2014-12-16 13:47 UTC #6 lore raw

PATCH v2 2/3 gadget: f_thor: check pointers before use in download_tail()

Hi Przemyslaw,

Quoted 43 lines

Some pointers in function download_tail() were not checked before the use. This could possibly cause the data abort. To avoid this, check if the pointers are not null is added.

Signed-off-by Przemyslaw Marczak <[email protected]>

---
Change v2:
- download_tail(): change printf() to error()

---
 drivers/usb/gadget/f_thor.c | 16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)

drivers/usb/gadget/f_thor.c +14 -2

index 78519fa..2d0410d 100644
@@ -205,12 +205,24 @@ static long long int download_head(unsigned
 long long total, 
 static int download_tail(long long int left, int cnt)
 {
-	struct dfu_entity *dfu_entity =
 dfu_get_entity(alt_setting_num);
-	void *transfer_buffer = dfu_get_buf(dfu_entity);
+	struct dfu_entity *dfu_entity;
+	void *transfer_buffer;
 	int ret;
 
 	debug("%s: left: %llu cnt: %d\n", __func__, left, cnt);
 
+	dfu_entity = dfu_get_entity(alt_setting_num);
+	if (!dfu_entity) {
+		error("Alt setting: %d entity not found!\n",
 alt_setting_num);
+		return -ENOENT;
+	}
+
+	transfer_buffer = dfu_get_buf(dfu_entity);
+	if (!transfer_buffer) {
+		error("Transfer buffer not allocated!");
+		return -ENXIO;
+	}
+
 	if (left) {

Quoted 2 lines

ret = dfu_write(dfu_entity, transfer_buffer, left,
cnt++); if (ret) {

Applied to u-boot-dfu, thanks!

Signature · 5 lines

Best regards,

Lukasz Majewski

Samsung R&D Institute Poland (SRPOL) | Linux Platform Group

Przemyslaw Marczak

author

2014-12-15 09:34 UTC #7 lore raw

PATCH v2 3/3 dfu: dfu_get_buf: check the value of env dfu_bufsiz before use

In reply to Przemyslaw Marczak

In function dfu_get_buf(), the size of allocated buffer could be defined by the env variable. The size from this variable was passed for memalign() without checking its value. And the the memalign will return non null pointer for size 0.

This could possibly cause data abort, so now the value of var is checked before use. And if this variable is set to 0 then the default size will be used.

This commit also changes the base passed to simple_strtoul() to 0. Now decimal and hex values can be used for the variable dfu_bufsiz.

Signed-off-by Przemyslaw Marczak <[email protected]>

---
Change v2:
- new patch

---
 drivers/dfu/dfu.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

drivers/dfu/dfu.c +6 -2

index c0aba6e..49abd85 100644
@@ -111,8 +111,12 @@ unsigned char *dfu_get_buf(struct dfu_entity *dfu)
 		return dfu_buf;
 
 	s = getenv("dfu_bufsiz");
-	dfu_buf_size = s ? (unsigned long)simple_strtol(s, NULL, 16) :
-			CONFIG_SYS_DFU_DATA_BUF_SIZE;
+	if (s)
+		dfu_buf_size = (unsigned long)simple_strtol(s, NULL, 0);
+
+	if (!s || !dfu_buf_size)
+		dfu_buf_size = CONFIG_SYS_DFU_DATA_BUF_SIZE;
+
 	if (dfu->max_buf_size && dfu_buf_size > dfu->max_buf_size)
 		dfu_buf_size = dfu->max_buf_size;

Signature · 1 line

1.9.1

Lukasz Majewski

2014-12-16 13:48 UTC #8 lore raw

PATCH v2 3/3 dfu: dfu_get_buf: check the value of env dfu_bufsiz before use

Hi Przemyslaw,

Quoted 36 lines

In function dfu_get_buf(), the size of allocated buffer could be defined by the env variable. The size from this variable was passed for memalign() without checking its value. And the the memalign will return non null pointer for size 0.
This could possibly cause data abort, so now the value of var is checked before use. And if this variable is set to 0 then the default size will be used.
This commit also changes the base passed to simple_strtoul() to 0. Now decimal and hex values can be used for the variable dfu_bufsiz.
Signed-off-by Przemyslaw Marczak <[email protected]>
---
Change v2:
- new patch
---
 drivers/dfu/dfu.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)
drivers/dfu/dfu.c +0 -1
index c0aba6e..49abd85 100644
@@ -111,8 +111,12 @@ unsigned char *dfu_get_buf(struct dfu_entity
 *dfu) return dfu_buf;
 
 	s = getenv("dfu_bufsiz");
-	dfu_buf_size = s ? (unsigned long)simple_strtol(s, NULL,
16) :
drivers/dfu/dfu.c +2 -1
-			CONFIG_SYS_DFU_DATA_BUF_SIZE;
+	if (s)
+		dfu_buf_size = (unsigned long)simple_strtol(s, NULL,
0); +

drivers/dfu/dfu.c unsigned char *dfu_get_buf(struct dfu_entity *dfu) · line 121

Quoted 5 lines

drivers/dfu/dfu.c +3 -0

+	if (!s || !dfu_buf_size)
+		dfu_buf_size = CONFIG_SYS_DFU_DATA_BUF_SIZE;
+
 	if (dfu->max_buf_size && dfu_buf_size > dfu->max_buf_size)
 		dfu_buf_size = dfu->max_buf_size;

Applied to u-boot-dfu, thanks!

Signature · 5 lines

Best regards,

Lukasz Majewski

Samsung R&D Institute Poland (SRPOL) | Linux Platform Group

Marek Vasut

2014-12-16 15:09 UTC #9 lore raw

PATCH v2 3/3 dfu: dfu_get_buf: check the value of env dfu_bufsiz before use

On Tuesday, December 16, 2014 at 02:48:46 PM, Lukasz Majewski wrote:
[...]

Quoted 1 line

Applied to u-boot-dfu, thanks!

Hi,

Will you have any PR for me for this MW please ? If so, when ?

Best regards,
Marek Vasut

Lukasz Majewski

2014-12-16 16:07 UTC #10 lore raw

PATCH v2 3/3 dfu: dfu_get_buf: check the value of env dfu_bufsiz before use

Hi Marek,

Quoted 7 lines · Lukasz Majewski

[...]
Quoted 1 line
Applied to u-boot-dfu, thanks!
Hi,
Will you have any PR for me for this MW please ? If so, when ?

Some fixes and clean ups I hope. By the end of current week.

Quoted 3 lines

Best regards,
Marek Vasut

Signature · 5 lines

Best regards,

Lukasz Majewski

Samsung R&D Institute Poland (SRPOL) | Linux Platform Group

Marek Vasut

2014-12-16 17:01 UTC #11 lore raw

PATCH v2 3/3 dfu: dfu_get_buf: check the value of env dfu_bufsiz before use

Quoted 5 lines · Lukasz Majewski

Hi Marek,
Quoted 3 lines · Lukasz Majewski
[...]

Quoted 7 lines

Quoted 5 lines
Quoted 1 line
Applied to u-boot-dfu, thanks!
Hi,
Will you have any PR for me for this MW please ? If so, when ?
Some fixes and clean ups I hope. By the end of current week.

I pushed an up-to-date u-boot-usb/master tree, so you can use the up-to-date code.

Hope that helps!

Thanks!

Best regards,
Marek Vasut

Lukasz Majewski

2014-12-16 13:47 UTC #12 lore raw

PATCH v2 1/3 dfu: mmc: check if mmc device exists in mmc_block_op()

In reply to Przemyslaw Marczak

Hi Przemyslaw,

Quoted 35 lines

The function mmc_block_op() is the last function before the physicall data write, but the mmc device pointer is not checked. If mmc device not exists, then data abort will occur. To avoid this, first the mmc device pointer is checked.
Signed-off-by Przemyslaw Marczak <[email protected]>
---
Change v2:
- mmc_block_op(): change printf() to error()
---
 drivers/dfu/dfu_mmc.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)
drivers/dfu/dfu_mmc.c +7 -1
index 72fa03e..62d72fe 100644
@@ -40,10 +40,16 @@ static int mmc_access_part(struct dfu_entity
 *dfu, struct mmc *mmc, int part) static int mmc_block_op(enum dfu_op
 op, struct dfu_entity *dfu, u64 offset, void *buf, long *len)
 {
-	struct mmc *mmc = find_mmc_device(dfu->data.mmc.dev_num);
+	struct mmc *mmc;
 	u32 blk_start, blk_count, n = 0;
 	int ret, part_num_bkp = 0;
 
+	mmc = find_mmc_device(dfu->data.mmc.dev_num);
+	if (!mmc) {
+		error("Device MMC %d - not found!",
 dfu->data.mmc.dev_num);
+		return -ENODEV;
+	}
+
 	/*
 	 * We must ensure that we work in lba_blk_size chunks, so

Quoted 2 lines

ALIGN
* this value.

Applied to u-boot-dfu, thanks!

Signature · 5 lines

Best regards,

Lukasz Majewski

Samsung R&D Institute Poland (SRPOL) | Linux Platform Group