[U-Boot] [PATCH 1/2] dfu: mmc: check if mmc device exists in mmc_block_op()

[U-Boot] [PATCH 1/2] dfu: mmc: check if mmc device exists in mmc_block_op()

[Przemyslaw Marczak]

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 <p.marczak@samsung.com>
---
 drivers/dfu/dfu_mmc.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/dfu/dfu_mmc.c b/drivers/dfu/dfu_mmc.c
index 72fa03e..41ac188 100644
--- a/drivers/dfu/dfu_mmc.c
+++ b/drivers/dfu/dfu_mmc.c
@@ -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.
-- 
1.9.1

[U-Boot] [PATCH 2/2] gadget: f_thor: check pointers before use in download_tail()

[Przemyslaw Marczak]

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 <p.marczak@samsung.com>
---
 drivers/usb/gadget/f_thor.c | 16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/gadget/f_thor.c b/drivers/usb/gadget/f_thor.c
index 78519fa..9f77ba6 100644
--- a/drivers/usb/gadget/f_thor.c
+++ b/drivers/usb/gadget/f_thor.c
@@ -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) {
-- 
1.9.1

[U-Boot] [PATCH 1/2] dfu: mmc: check if mmc device exists in mmc_block_op()

[Lukasz Majewski]

Hi Przemyslaw,

> 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 <p.marczak@samsung.com>
> ---
>  drivers/dfu/dfu_mmc.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/dfu/dfu_mmc.c b/drivers/dfu/dfu_mmc.c
> index 72fa03e..41ac188 100644
> --- a/drivers/dfu/dfu_mmc.c
> +++ b/drivers/dfu/dfu_mmc.c
> @@ -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);

Please change printf() -> error();

> +		return -ENODEV;
> +	}
> +
>  	/*
>  	 * We must ensure that we work in lba_blk_size chunks, so
> ALIGN
>  	 * this value.



-- 
Best regards,

Lukasz Majewski

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

[U-Boot] [PATCH 2/2] gadget: f_thor: check pointers before use in download_tail()

[Lukasz Majewski]

Hi Przemyslaw,

> 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 <p.marczak@samsung.com>
> ---
>  drivers/usb/gadget/f_thor.c | 16 ++++++++++++++--
>  1 file changed, 14 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/usb/gadget/f_thor.c b/drivers/usb/gadget/f_thor.c
> index 78519fa..9f77ba6 100644
> --- a/drivers/usb/gadget/f_thor.c
> +++ b/drivers/usb/gadget/f_thor.c
> @@ -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);

	Please change printf() -> error()

> +		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) {



-- 
Best regards,

Lukasz Majewski

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

[U-Boot] [Patch V2 1/3] dfu: mmc: check if mmc device exists in mmc_block_op()

[Przemyslaw Marczak]

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 <p.marczak@samsung.com>
---
Change v2:
- mmc_block_op(): change printf() to error()
---
 drivers/dfu/dfu_mmc.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/dfu/dfu_mmc.c b/drivers/dfu/dfu_mmc.c
index 72fa03e..62d72fe 100644
--- a/drivers/dfu/dfu_mmc.c
+++ b/drivers/dfu/dfu_mmc.c
@@ -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.
-- 
1.9.1

[U-Boot] [Patch V2 2/3] gadget: f_thor: check pointers before use in download_tail()

[Przemyslaw Marczak]

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 <p.marczak@samsung.com>
---
Change v2:
- download_tail(): change printf() to error()
---
 drivers/usb/gadget/f_thor.c | 16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/gadget/f_thor.c b/drivers/usb/gadget/f_thor.c
index 78519fa..2d0410d 100644
--- a/drivers/usb/gadget/f_thor.c
+++ b/drivers/usb/gadget/f_thor.c
@@ -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) {
-- 
1.9.1

[U-Boot] [Patch V2 3/3] dfu: dfu_get_buf: check the value of env dfu_bufsiz before use

[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 <p.marczak@samsung.com>
---
Change v2:
- new patch
---
 drivers/dfu/dfu.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/dfu/dfu.c b/drivers/dfu/dfu.c
index c0aba6e..49abd85 100644
--- a/drivers/dfu/dfu.c
+++ b/drivers/dfu/dfu.c
@@ -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;
 
-- 
1.9.1

[U-Boot] [Patch V2 1/3] dfu: mmc: check if mmc device exists in mmc_block_op()

[Lukasz Majewski]

Hi Przemyslaw,

> 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 <p.marczak@samsung.com>
> ---
> Change v2:
> - mmc_block_op(): change printf() to error()
> ---
>  drivers/dfu/dfu_mmc.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/dfu/dfu_mmc.c b/drivers/dfu/dfu_mmc.c
> index 72fa03e..62d72fe 100644
> --- a/drivers/dfu/dfu_mmc.c
> +++ b/drivers/dfu/dfu_mmc.c
> @@ -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.

Applied to u-boot-dfu, thanks!

-- 
Best regards,

Lukasz Majewski

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

[U-Boot] [Patch V2 2/3] gadget: f_thor: check pointers before use in download_tail()

[Lukasz Majewski]

Hi Przemyslaw,

> 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 <p.marczak@samsung.com>
> ---
> Change v2:
> - download_tail(): change printf() to error()
> ---
>  drivers/usb/gadget/f_thor.c | 16 ++++++++++++++--
>  1 file changed, 14 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/usb/gadget/f_thor.c b/drivers/usb/gadget/f_thor.c
> index 78519fa..2d0410d 100644
> --- a/drivers/usb/gadget/f_thor.c
> +++ b/drivers/usb/gadget/f_thor.c
> @@ -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) {

Applied to u-boot-dfu, thanks!

-- 
Best regards,

Lukasz Majewski

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

[U-Boot] [Patch V2 3/3] dfu: dfu_get_buf: check the value of env dfu_bufsiz before use

[Lukasz Majewski]

Hi Przemyslaw,

> 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 <p.marczak@samsung.com>
> ---
> Change v2:
> - new patch
> ---
>  drivers/dfu/dfu.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/dfu/dfu.c b/drivers/dfu/dfu.c
> index c0aba6e..49abd85 100644
> --- a/drivers/dfu/dfu.c
> +++ b/drivers/dfu/dfu.c
> @@ -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;
>  

Applied to u-boot-dfu, thanks!

-- 
Best regards,

Lukasz Majewski

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

[U-Boot] [Patch V2 3/3] dfu: dfu_get_buf: check the value of env dfu_bufsiz before use

[Marek Vasut]

On Tuesday, December 16, 2014 at 02:48:46 PM, Lukasz Majewski wrote:
[...]
> 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

[U-Boot] [Patch V2 3/3] dfu: dfu_get_buf: check the value of env dfu_bufsiz before use

[Lukasz Majewski]

Hi Marek,

> On Tuesday, December 16, 2014 at 02:48:46 PM, Lukasz Majewski wrote:
> [...]
> > 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.

> 
> Best regards,
> Marek Vasut



-- 
Best regards,

Lukasz Majewski

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

[U-Boot] [Patch V2 3/3] dfu: dfu_get_buf: check the value of env dfu_bufsiz before use

[Marek Vasut]

On Tuesday, December 16, 2014 at 05:07:06 PM, Lukasz Majewski wrote:
> Hi Marek,
> 
> > On Tuesday, December 16, 2014 at 02:48:46 PM, Lukasz Majewski wrote:
> > [...]
> > 
> > > 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