* [PATCH] staging: greybus: fix fw is NULL but dereferenced.
@ 2020-01-26 8:31 ` Saurav Girepunje
0 siblings, 0 replies; 18+ messages in thread
From: Saurav Girepunje @ 2020-01-26 8:31 UTC (permalink / raw)
To: vireshk, johan, elder, gregkh, greybus-dev, devel, linux-kernel
Cc: saurav.girepunje
Fix the warning reported by cocci check.
Changes:
In queue_work fw dereference before it actually get assigned.
move queue_work before gb_bootrom_set_timeout.
As gb_bootrom_get_firmware () return NEXT_REQ_READY_TO_BOOT
only when there is no error and offset + size is actually equal
to fw->size. So initialized next_request to NEXT_REQ_GET_FIRMWARE
for return in other case.
Signed-off-by: Saurav Girepunje <saurav.girepunje@gmail.com>
---
drivers/staging/greybus/bootrom.c | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/drivers/staging/greybus/bootrom.c b/drivers/staging/greybus/bootrom.c
index a8efb86..f54514e 100644
--- a/drivers/staging/greybus/bootrom.c
+++ b/drivers/staging/greybus/bootrom.c
@@ -246,7 +246,7 @@ static int gb_bootrom_get_firmware(struct gb_operation *op)
struct gb_bootrom_get_firmware_response *firmware_response;
struct device *dev = &op->connection->bundle->dev;
unsigned int offset, size;
- enum next_request_type next_request;
+ enum next_request_type next_request = NEXT_REQ_GET_FIRMWARE;
int ret = 0;
/* Disable timeouts */
@@ -296,13 +296,11 @@ static int gb_bootrom_get_firmware(struct gb_operation *op)
unlock:
mutex_unlock(&bootrom->mutex);
-queue_work:
/* Refresh timeout */
if (!ret && (offset + size == fw->size))
next_request = NEXT_REQ_READY_TO_BOOT;
- else
- next_request = NEXT_REQ_GET_FIRMWARE;
+queue_work:
gb_bootrom_set_timeout(bootrom, next_request, NEXT_REQ_TIMEOUT_MS);
return ret;
--
1.9.1
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH] staging: greybus: fix fw is NULL but dereferenced.
@ 2020-01-26 8:31 ` Saurav Girepunje
0 siblings, 0 replies; 18+ messages in thread
From: Saurav Girepunje @ 2020-01-26 8:31 UTC (permalink / raw)
To: vireshk, johan, elder, gregkh, greybus-dev, devel, linux-kernel
Cc: saurav.girepunje
Fix the warning reported by cocci check.
Changes:
In queue_work fw dereference before it actually get assigned.
move queue_work before gb_bootrom_set_timeout.
As gb_bootrom_get_firmware () return NEXT_REQ_READY_TO_BOOT
only when there is no error and offset + size is actually equal
to fw->size. So initialized next_request to NEXT_REQ_GET_FIRMWARE
for return in other case.
Signed-off-by: Saurav Girepunje <saurav.girepunje@gmail.com>
---
drivers/staging/greybus/bootrom.c | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/drivers/staging/greybus/bootrom.c b/drivers/staging/greybus/bootrom.c
index a8efb86..f54514e 100644
--- a/drivers/staging/greybus/bootrom.c
+++ b/drivers/staging/greybus/bootrom.c
@@ -246,7 +246,7 @@ static int gb_bootrom_get_firmware(struct gb_operation *op)
struct gb_bootrom_get_firmware_response *firmware_response;
struct device *dev = &op->connection->bundle->dev;
unsigned int offset, size;
- enum next_request_type next_request;
+ enum next_request_type next_request = NEXT_REQ_GET_FIRMWARE;
int ret = 0;
/* Disable timeouts */
@@ -296,13 +296,11 @@ static int gb_bootrom_get_firmware(struct gb_operation *op)
unlock:
mutex_unlock(&bootrom->mutex);
-queue_work:
/* Refresh timeout */
if (!ret && (offset + size == fw->size))
next_request = NEXT_REQ_READY_TO_BOOT;
- else
- next_request = NEXT_REQ_GET_FIRMWARE;
+queue_work:
gb_bootrom_set_timeout(bootrom, next_request, NEXT_REQ_TIMEOUT_MS);
return ret;
--
1.9.1
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH] staging: greybus: fix fw is NULL but dereferenced.
2020-01-26 8:31 ` Saurav Girepunje
@ 2020-01-26 11:04 ` Johan Hovold
-1 siblings, 0 replies; 18+ messages in thread
From: Johan Hovold @ 2020-01-26 11:04 UTC (permalink / raw)
To: Saurav Girepunje
Cc: vireshk, johan, elder, gregkh, greybus-dev, devel, linux-kernel,
saurav.girepunje
On Sun, Jan 26, 2020 at 02:01:30PM +0530, Saurav Girepunje wrote:
> Fix the warning reported by cocci check.
>
> Changes:
>
> In queue_work fw dereference before it actually get assigned.
> move queue_work before gb_bootrom_set_timeout.
Nope. As I said yesterday, you need to verify the output of any static
checkers you use.
The code may be unnecessarily subtle, but there's no way fw can be
dereferenced before being initialised currently.
> -queue_work:
> /* Refresh timeout */
> if (!ret && (offset + size == fw->size))
Specifically, the second operand is never evaluated if ret is non-zero.
> next_request = NEXT_REQ_READY_TO_BOOT;
> - else
> - next_request = NEXT_REQ_GET_FIRMWARE;
>
> +queue_work:
> gb_bootrom_set_timeout(bootrom, next_request, NEXT_REQ_TIMEOUT_MS);
>
> return ret;
Johan
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] staging: greybus: fix fw is NULL but dereferenced.
@ 2020-01-26 11:04 ` Johan Hovold
0 siblings, 0 replies; 18+ messages in thread
From: Johan Hovold @ 2020-01-26 11:04 UTC (permalink / raw)
To: Saurav Girepunje
Cc: devel, elder, vireshk, johan, linux-kernel, greybus-dev,
saurav.girepunje, gregkh
On Sun, Jan 26, 2020 at 02:01:30PM +0530, Saurav Girepunje wrote:
> Fix the warning reported by cocci check.
>
> Changes:
>
> In queue_work fw dereference before it actually get assigned.
> move queue_work before gb_bootrom_set_timeout.
Nope. As I said yesterday, you need to verify the output of any static
checkers you use.
The code may be unnecessarily subtle, but there's no way fw can be
dereferenced before being initialised currently.
> -queue_work:
> /* Refresh timeout */
> if (!ret && (offset + size == fw->size))
Specifically, the second operand is never evaluated if ret is non-zero.
> next_request = NEXT_REQ_READY_TO_BOOT;
> - else
> - next_request = NEXT_REQ_GET_FIRMWARE;
>
> +queue_work:
> gb_bootrom_set_timeout(bootrom, next_request, NEXT_REQ_TIMEOUT_MS);
>
> return ret;
Johan
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] staging: greybus: fix fw is NULL but dereferenced.
2020-01-26 8:31 ` Saurav Girepunje
@ 2020-01-26 18:30 ` Greg KH
-1 siblings, 0 replies; 18+ messages in thread
From: Greg KH @ 2020-01-26 18:30 UTC (permalink / raw)
To: Saurav Girepunje
Cc: vireshk, johan, elder, greybus-dev, devel, linux-kernel,
saurav.girepunje
On Sun, Jan 26, 2020 at 02:01:30PM +0530, Saurav Girepunje wrote:
> Fix the warning reported by cocci check.
What is "cocci check"?
> Changes:
>
Why add that line?
> In queue_work fw dereference before it actually get assigned.
> move queue_work before gb_bootrom_set_timeout.
>
> As gb_bootrom_get_firmware () return NEXT_REQ_READY_TO_BOOT
> only when there is no error and offset + size is actually equal
> to fw->size. So initialized next_request to NEXT_REQ_GET_FIRMWARE
> for return in other case.
>
> Signed-off-by: Saurav Girepunje <saurav.girepunje@gmail.com>
> ---
> drivers/staging/greybus/bootrom.c | 6 ++----
> 1 file changed, 2 insertions(+), 4 deletions(-)
As Johan said, there are a lot of really bad "static checking"
tools out there that can not properly parse C code. Always verify by
hand what the tools said is wrong, really is an issue before sending a
patch out for something that is not correct. This looks like you need
to use a better tool.
greg k-h
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] staging: greybus: fix fw is NULL but dereferenced.
@ 2020-01-26 18:30 ` Greg KH
0 siblings, 0 replies; 18+ messages in thread
From: Greg KH @ 2020-01-26 18:30 UTC (permalink / raw)
To: Saurav Girepunje
Cc: devel, elder, vireshk, johan, linux-kernel, greybus-dev,
saurav.girepunje
On Sun, Jan 26, 2020 at 02:01:30PM +0530, Saurav Girepunje wrote:
> Fix the warning reported by cocci check.
What is "cocci check"?
> Changes:
>
Why add that line?
> In queue_work fw dereference before it actually get assigned.
> move queue_work before gb_bootrom_set_timeout.
>
> As gb_bootrom_get_firmware () return NEXT_REQ_READY_TO_BOOT
> only when there is no error and offset + size is actually equal
> to fw->size. So initialized next_request to NEXT_REQ_GET_FIRMWARE
> for return in other case.
>
> Signed-off-by: Saurav Girepunje <saurav.girepunje@gmail.com>
> ---
> drivers/staging/greybus/bootrom.c | 6 ++----
> 1 file changed, 2 insertions(+), 4 deletions(-)
As Johan said, there are a lot of really bad "static checking"
tools out there that can not properly parse C code. Always verify by
hand what the tools said is wrong, really is an issue before sending a
patch out for something that is not correct. This looks like you need
to use a better tool.
greg k-h
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH] staging: greybus: fix fw is NULL but dereferenced
@ 2021-03-25 10:19 ` Jian Dong
0 siblings, 0 replies; 18+ messages in thread
From: Jian Dong @ 2021-03-25 10:19 UTC (permalink / raw)
To: vireshk, johan, elder, gregkh
Cc: greybus-dev, devel, linux-kernel, huyue2, Jian Dong
From: Jian Dong <dongjian@yulong.com>
fixes coccicheck Error:
drivers/staging/greybus/bootrom.c:301:41-45: ERROR:
fw is NULL but dereferenced.
if procedure goto label directly, ret will be nefative, so the fw is NULL
and the if(condition) end with dereferenced fw. let's fix it.
Signed-off-by: Jian Dong <dongjian@yulong.com>
---
drivers/staging/greybus/bootrom.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/staging/greybus/bootrom.c b/drivers/staging/greybus/bootrom.c
index a8efb86..0439efa 100644
--- a/drivers/staging/greybus/bootrom.c
+++ b/drivers/staging/greybus/bootrom.c
@@ -246,7 +246,7 @@ static int gb_bootrom_get_firmware(struct gb_operation *op)
struct gb_bootrom_get_firmware_response *firmware_response;
struct device *dev = &op->connection->bundle->dev;
unsigned int offset, size;
- enum next_request_type next_request;
+ enum next_request_type next_request = NEXT_REQ_GET_FIRMWARE;
int ret = 0;
/* Disable timeouts */
@@ -298,10 +298,10 @@ static int gb_bootrom_get_firmware(struct gb_operation *op)
queue_work:
/* Refresh timeout */
- if (!ret && (offset + size == fw->size))
- next_request = NEXT_REQ_READY_TO_BOOT;
- else
+ if (!!ret)
next_request = NEXT_REQ_GET_FIRMWARE;
+ else if (offset + size == fw->size)
+ next_request = NEXT_REQ_READY_TO_BOOT;
gb_bootrom_set_timeout(bootrom, next_request, NEXT_REQ_TIMEOUT_MS);
--
1.9.1
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH] staging: greybus: fix fw is NULL but dereferenced
@ 2021-03-25 10:19 ` Jian Dong
0 siblings, 0 replies; 18+ messages in thread
From: Jian Dong @ 2021-03-25 10:19 UTC (permalink / raw)
To: vireshk, johan, elder, gregkh
Cc: devel, greybus-dev, huyue2, Jian Dong, linux-kernel
From: Jian Dong <dongjian@yulong.com>
fixes coccicheck Error:
drivers/staging/greybus/bootrom.c:301:41-45: ERROR:
fw is NULL but dereferenced.
if procedure goto label directly, ret will be nefative, so the fw is NULL
and the if(condition) end with dereferenced fw. let's fix it.
Signed-off-by: Jian Dong <dongjian@yulong.com>
---
drivers/staging/greybus/bootrom.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/staging/greybus/bootrom.c b/drivers/staging/greybus/bootrom.c
index a8efb86..0439efa 100644
--- a/drivers/staging/greybus/bootrom.c
+++ b/drivers/staging/greybus/bootrom.c
@@ -246,7 +246,7 @@ static int gb_bootrom_get_firmware(struct gb_operation *op)
struct gb_bootrom_get_firmware_response *firmware_response;
struct device *dev = &op->connection->bundle->dev;
unsigned int offset, size;
- enum next_request_type next_request;
+ enum next_request_type next_request = NEXT_REQ_GET_FIRMWARE;
int ret = 0;
/* Disable timeouts */
@@ -298,10 +298,10 @@ static int gb_bootrom_get_firmware(struct gb_operation *op)
queue_work:
/* Refresh timeout */
- if (!ret && (offset + size == fw->size))
- next_request = NEXT_REQ_READY_TO_BOOT;
- else
+ if (!!ret)
next_request = NEXT_REQ_GET_FIRMWARE;
+ else if (offset + size == fw->size)
+ next_request = NEXT_REQ_READY_TO_BOOT;
gb_bootrom_set_timeout(bootrom, next_request, NEXT_REQ_TIMEOUT_MS);
--
1.9.1
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH] staging: greybus: fix fw is NULL but dereferenced
2021-03-25 10:19 ` Jian Dong
@ 2021-03-25 10:29 ` Greg KH
-1 siblings, 0 replies; 18+ messages in thread
From: Greg KH @ 2021-03-25 10:29 UTC (permalink / raw)
To: Jian Dong
Cc: vireshk, johan, elder, greybus-dev, devel, linux-kernel, huyue2,
Jian Dong
On Thu, Mar 25, 2021 at 06:19:26PM +0800, Jian Dong wrote:
> From: Jian Dong <dongjian@yulong.com>
>
> fixes coccicheck Error:
>
> drivers/staging/greybus/bootrom.c:301:41-45: ERROR:
> fw is NULL but dereferenced.
>
> if procedure goto label directly, ret will be nefative, so the fw is NULL
> and the if(condition) end with dereferenced fw. let's fix it.
Why is this all indented a space?
>
> Signed-off-by: Jian Dong <dongjian@yulong.com>
> ---
> drivers/staging/greybus/bootrom.c | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/staging/greybus/bootrom.c b/drivers/staging/greybus/bootrom.c
> index a8efb86..0439efa 100644
> --- a/drivers/staging/greybus/bootrom.c
> +++ b/drivers/staging/greybus/bootrom.c
> @@ -246,7 +246,7 @@ static int gb_bootrom_get_firmware(struct gb_operation *op)
> struct gb_bootrom_get_firmware_response *firmware_response;
> struct device *dev = &op->connection->bundle->dev;
> unsigned int offset, size;
> - enum next_request_type next_request;
> + enum next_request_type next_request = NEXT_REQ_GET_FIRMWARE;
> int ret = 0;
>
> /* Disable timeouts */
> @@ -298,10 +298,10 @@ static int gb_bootrom_get_firmware(struct gb_operation *op)
>
> queue_work:
> /* Refresh timeout */
> - if (!ret && (offset + size == fw->size))
> - next_request = NEXT_REQ_READY_TO_BOOT;
> - else
> + if (!!ret)
That is hard to understand, please make this more obvious.
thanks,
greg k-h
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] staging: greybus: fix fw is NULL but dereferenced
@ 2021-03-25 10:29 ` Greg KH
0 siblings, 0 replies; 18+ messages in thread
From: Greg KH @ 2021-03-25 10:29 UTC (permalink / raw)
To: Jian Dong
Cc: devel, elder, vireshk, johan, linux-kernel, greybus-dev, huyue2,
Jian Dong
On Thu, Mar 25, 2021 at 06:19:26PM +0800, Jian Dong wrote:
> From: Jian Dong <dongjian@yulong.com>
>
> fixes coccicheck Error:
>
> drivers/staging/greybus/bootrom.c:301:41-45: ERROR:
> fw is NULL but dereferenced.
>
> if procedure goto label directly, ret will be nefative, so the fw is NULL
> and the if(condition) end with dereferenced fw. let's fix it.
Why is this all indented a space?
>
> Signed-off-by: Jian Dong <dongjian@yulong.com>
> ---
> drivers/staging/greybus/bootrom.c | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/staging/greybus/bootrom.c b/drivers/staging/greybus/bootrom.c
> index a8efb86..0439efa 100644
> --- a/drivers/staging/greybus/bootrom.c
> +++ b/drivers/staging/greybus/bootrom.c
> @@ -246,7 +246,7 @@ static int gb_bootrom_get_firmware(struct gb_operation *op)
> struct gb_bootrom_get_firmware_response *firmware_response;
> struct device *dev = &op->connection->bundle->dev;
> unsigned int offset, size;
> - enum next_request_type next_request;
> + enum next_request_type next_request = NEXT_REQ_GET_FIRMWARE;
> int ret = 0;
>
> /* Disable timeouts */
> @@ -298,10 +298,10 @@ static int gb_bootrom_get_firmware(struct gb_operation *op)
>
> queue_work:
> /* Refresh timeout */
> - if (!ret && (offset + size == fw->size))
> - next_request = NEXT_REQ_READY_TO_BOOT;
> - else
> + if (!!ret)
That is hard to understand, please make this more obvious.
thanks,
greg k-h
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] staging: greybus: fix fw is NULL but dereferenced
2021-03-25 10:29 ` Greg KH
@ 2021-03-25 11:03 ` Jian Dong
-1 siblings, 0 replies; 18+ messages in thread
From: Jian Dong @ 2021-03-25 11:03 UTC (permalink / raw)
To: Greg KH
Cc: vireshk, johan, elder, greybus-dev, devel, linux-kernel, Jian Dong
On Thu, 25 Mar 2021 11:29:06 +0100
Greg KH <gregkh@linuxfoundation.org> wrote:
> On Thu, Mar 25, 2021 at 06:19:26PM +0800, Jian Dong wrote:
> > From: Jian Dong <dongjian@yulong.com>
> >
> > fixes coccicheck Error:
> >
> > drivers/staging/greybus/bootrom.c:301:41-45: ERROR:
> > fw is NULL but dereferenced.
> >
> > if procedure goto label directly, ret will be nefative, so the fw
> > is NULL and the if(condition) end with dereferenced fw. let's fix
> > it.
>
> Why is this all indented a space?
this maybe caused by my terminal, I will take notice next time.
>
> >
> > Signed-off-by: Jian Dong <dongjian@yulong.com>
> > ---
> > drivers/staging/greybus/bootrom.c | 8 ++++----
> > 1 file changed, 4 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/staging/greybus/bootrom.c
> > b/drivers/staging/greybus/bootrom.c index a8efb86..0439efa 100644
> > --- a/drivers/staging/greybus/bootrom.c
> > +++ b/drivers/staging/greybus/bootrom.c
> > @@ -246,7 +246,7 @@ static int gb_bootrom_get_firmware(struct
> > gb_operation *op) struct gb_bootrom_get_firmware_response
> > *firmware_response; struct device *dev =
> > &op->connection->bundle->dev; unsigned int offset, size;
> > - enum next_request_type next_request;
> > + enum next_request_type next_request =
> > NEXT_REQ_GET_FIRMWARE; int ret = 0;
> >
> > /* Disable timeouts */
> > @@ -298,10 +298,10 @@ static int gb_bootrom_get_firmware(struct
> > gb_operation *op)
> > queue_work:
> > /* Refresh timeout */
> > - if (!ret && (offset + size == fw->size))
> > - next_request = NEXT_REQ_READY_TO_BOOT;
> > - else
> > + if (!!ret)
>
> That is hard to understand, please make this more obvious.
>
if (A && B) else (!A || !B)
So, when ret is NON-ZERO, set next_request as GET_FIRMWARE, else set
READ_TO_BOOT. but if second express is flase, next_request still
need be set as GET_FIRMWARE, So, I initialze it as GET_FIRMWARE.
this is will keep consistent with the origin conditional express:
both ret is ZERO and second express TRUE, then set as READ_TO_BOOT,
else set as GET_FIRMWARE.
> thanks,
>
> greg k-h
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] staging: greybus: fix fw is NULL but dereferenced
@ 2021-03-25 11:03 ` Jian Dong
0 siblings, 0 replies; 18+ messages in thread
From: Jian Dong @ 2021-03-25 11:03 UTC (permalink / raw)
To: Greg KH
Cc: devel, elder, vireshk, johan, linux-kernel, greybus-dev, Jian Dong
On Thu, 25 Mar 2021 11:29:06 +0100
Greg KH <gregkh@linuxfoundation.org> wrote:
> On Thu, Mar 25, 2021 at 06:19:26PM +0800, Jian Dong wrote:
> > From: Jian Dong <dongjian@yulong.com>
> >
> > fixes coccicheck Error:
> >
> > drivers/staging/greybus/bootrom.c:301:41-45: ERROR:
> > fw is NULL but dereferenced.
> >
> > if procedure goto label directly, ret will be nefative, so the fw
> > is NULL and the if(condition) end with dereferenced fw. let's fix
> > it.
>
> Why is this all indented a space?
this maybe caused by my terminal, I will take notice next time.
>
> >
> > Signed-off-by: Jian Dong <dongjian@yulong.com>
> > ---
> > drivers/staging/greybus/bootrom.c | 8 ++++----
> > 1 file changed, 4 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/staging/greybus/bootrom.c
> > b/drivers/staging/greybus/bootrom.c index a8efb86..0439efa 100644
> > --- a/drivers/staging/greybus/bootrom.c
> > +++ b/drivers/staging/greybus/bootrom.c
> > @@ -246,7 +246,7 @@ static int gb_bootrom_get_firmware(struct
> > gb_operation *op) struct gb_bootrom_get_firmware_response
> > *firmware_response; struct device *dev =
> > &op->connection->bundle->dev; unsigned int offset, size;
> > - enum next_request_type next_request;
> > + enum next_request_type next_request =
> > NEXT_REQ_GET_FIRMWARE; int ret = 0;
> >
> > /* Disable timeouts */
> > @@ -298,10 +298,10 @@ static int gb_bootrom_get_firmware(struct
> > gb_operation *op)
> > queue_work:
> > /* Refresh timeout */
> > - if (!ret && (offset + size == fw->size))
> > - next_request = NEXT_REQ_READY_TO_BOOT;
> > - else
> > + if (!!ret)
>
> That is hard to understand, please make this more obvious.
>
if (A && B) else (!A || !B)
So, when ret is NON-ZERO, set next_request as GET_FIRMWARE, else set
READ_TO_BOOT. but if second express is flase, next_request still
need be set as GET_FIRMWARE, So, I initialze it as GET_FIRMWARE.
this is will keep consistent with the origin conditional express:
both ret is ZERO and second express TRUE, then set as READ_TO_BOOT,
else set as GET_FIRMWARE.
> thanks,
>
> greg k-h
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] staging: greybus: fix fw is NULL but dereferenced
2021-03-25 11:03 ` Jian Dong
@ 2021-03-25 11:32 ` Greg KH
-1 siblings, 0 replies; 18+ messages in thread
From: Greg KH @ 2021-03-25 11:32 UTC (permalink / raw)
To: Jian Dong
Cc: devel, elder, vireshk, johan, linux-kernel, greybus-dev, Jian Dong
On Thu, Mar 25, 2021 at 07:03:39PM +0800, Jian Dong wrote:
> On Thu, 25 Mar 2021 11:29:06 +0100
> Greg KH <gregkh@linuxfoundation.org> wrote:
>
> > On Thu, Mar 25, 2021 at 06:19:26PM +0800, Jian Dong wrote:
> > > From: Jian Dong <dongjian@yulong.com>
> > >
> > > fixes coccicheck Error:
> > >
> > > drivers/staging/greybus/bootrom.c:301:41-45: ERROR:
> > > fw is NULL but dereferenced.
> > >
> > > if procedure goto label directly, ret will be nefative, so the fw
> > > is NULL and the if(condition) end with dereferenced fw. let's fix
> > > it.
> >
> > Why is this all indented a space?
> this maybe caused by my terminal, I will take notice next time.
> >
> > >
> > > Signed-off-by: Jian Dong <dongjian@yulong.com>
> > > ---
> > > drivers/staging/greybus/bootrom.c | 8 ++++----
> > > 1 file changed, 4 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/drivers/staging/greybus/bootrom.c
> > > b/drivers/staging/greybus/bootrom.c index a8efb86..0439efa 100644
> > > --- a/drivers/staging/greybus/bootrom.c
> > > +++ b/drivers/staging/greybus/bootrom.c
> > > @@ -246,7 +246,7 @@ static int gb_bootrom_get_firmware(struct
> > > gb_operation *op) struct gb_bootrom_get_firmware_response
> > > *firmware_response; struct device *dev =
> > > &op->connection->bundle->dev; unsigned int offset, size;
> > > - enum next_request_type next_request;
> > > + enum next_request_type next_request =
> > > NEXT_REQ_GET_FIRMWARE; int ret = 0;
> > >
> > > /* Disable timeouts */
> > > @@ -298,10 +298,10 @@ static int gb_bootrom_get_firmware(struct
> > > gb_operation *op)
> > > queue_work:
> > > /* Refresh timeout */
> > > - if (!ret && (offset + size == fw->size))
> > > - next_request = NEXT_REQ_READY_TO_BOOT;
> > > - else
> > > + if (!!ret)
> >
> > That is hard to understand, please make this more obvious.
> >
> if (A && B) else (!A || !B)
>
> So, when ret is NON-ZERO, set next_request as GET_FIRMWARE, else set
> READ_TO_BOOT. but if second express is flase, next_request still
> need be set as GET_FIRMWARE, So, I initialze it as GET_FIRMWARE.
My point is:
if (!!ret)
is odd, and is the same thing as:
if (ret)
correct?
And the latter is the common kernel style, no need to be complex when
you do not need to.
Anyway, others have pointed out why this is incorrect, no need for
further discussion.
thanks,
greg k-h
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] staging: greybus: fix fw is NULL but dereferenced
@ 2021-03-25 11:32 ` Greg KH
0 siblings, 0 replies; 18+ messages in thread
From: Greg KH @ 2021-03-25 11:32 UTC (permalink / raw)
To: Jian Dong
Cc: devel, elder, vireshk, johan, linux-kernel, greybus-dev, Jian Dong
On Thu, Mar 25, 2021 at 07:03:39PM +0800, Jian Dong wrote:
> On Thu, 25 Mar 2021 11:29:06 +0100
> Greg KH <gregkh@linuxfoundation.org> wrote:
>
> > On Thu, Mar 25, 2021 at 06:19:26PM +0800, Jian Dong wrote:
> > > From: Jian Dong <dongjian@yulong.com>
> > >
> > > fixes coccicheck Error:
> > >
> > > drivers/staging/greybus/bootrom.c:301:41-45: ERROR:
> > > fw is NULL but dereferenced.
> > >
> > > if procedure goto label directly, ret will be nefative, so the fw
> > > is NULL and the if(condition) end with dereferenced fw. let's fix
> > > it.
> >
> > Why is this all indented a space?
> this maybe caused by my terminal, I will take notice next time.
> >
> > >
> > > Signed-off-by: Jian Dong <dongjian@yulong.com>
> > > ---
> > > drivers/staging/greybus/bootrom.c | 8 ++++----
> > > 1 file changed, 4 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/drivers/staging/greybus/bootrom.c
> > > b/drivers/staging/greybus/bootrom.c index a8efb86..0439efa 100644
> > > --- a/drivers/staging/greybus/bootrom.c
> > > +++ b/drivers/staging/greybus/bootrom.c
> > > @@ -246,7 +246,7 @@ static int gb_bootrom_get_firmware(struct
> > > gb_operation *op) struct gb_bootrom_get_firmware_response
> > > *firmware_response; struct device *dev =
> > > &op->connection->bundle->dev; unsigned int offset, size;
> > > - enum next_request_type next_request;
> > > + enum next_request_type next_request =
> > > NEXT_REQ_GET_FIRMWARE; int ret = 0;
> > >
> > > /* Disable timeouts */
> > > @@ -298,10 +298,10 @@ static int gb_bootrom_get_firmware(struct
> > > gb_operation *op)
> > > queue_work:
> > > /* Refresh timeout */
> > > - if (!ret && (offset + size == fw->size))
> > > - next_request = NEXT_REQ_READY_TO_BOOT;
> > > - else
> > > + if (!!ret)
> >
> > That is hard to understand, please make this more obvious.
> >
> if (A && B) else (!A || !B)
>
> So, when ret is NON-ZERO, set next_request as GET_FIRMWARE, else set
> READ_TO_BOOT. but if second express is flase, next_request still
> need be set as GET_FIRMWARE, So, I initialze it as GET_FIRMWARE.
My point is:
if (!!ret)
is odd, and is the same thing as:
if (ret)
correct?
And the latter is the common kernel style, no need to be complex when
you do not need to.
Anyway, others have pointed out why this is incorrect, no need for
further discussion.
thanks,
greg k-h
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] staging: greybus: fix fw is NULL but dereferenced
2021-03-25 10:19 ` Jian Dong
@ 2021-03-25 10:50 ` Dan Carpenter
-1 siblings, 0 replies; 18+ messages in thread
From: Dan Carpenter @ 2021-03-25 10:50 UTC (permalink / raw)
To: Jian Dong
Cc: vireshk, johan, elder, gregkh, devel, greybus-dev, huyue2,
Jian Dong, linux-kernel
The commit description is not clear but this patch doesn't change how
the code works, it just silences a static checker false positive.
Just ignore the false positive. Always just ignore static checkers
when they are wrong.
regards,
dan carpenter
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] staging: greybus: fix fw is NULL but dereferenced
@ 2021-03-25 10:50 ` Dan Carpenter
0 siblings, 0 replies; 18+ messages in thread
From: Dan Carpenter @ 2021-03-25 10:50 UTC (permalink / raw)
To: Jian Dong
Cc: devel, elder, vireshk, johan, linux-kernel, greybus-dev, huyue2,
gregkh, Jian Dong
The commit description is not clear but this patch doesn't change how
the code works, it just silences a static checker false positive.
Just ignore the false positive. Always just ignore static checkers
when they are wrong.
regards,
dan carpenter
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] staging: greybus: fix fw is NULL but dereferenced
2021-03-25 10:19 ` Jian Dong
@ 2021-03-25 10:51 ` Viresh Kumar
-1 siblings, 0 replies; 18+ messages in thread
From: Viresh Kumar @ 2021-03-25 10:51 UTC (permalink / raw)
To: Jian Dong
Cc: vireshk, johan, elder, gregkh, greybus-dev, devel, linux-kernel,
huyue2, Jian Dong
On 25-03-21, 18:19, Jian Dong wrote:
> From: Jian Dong <dongjian@yulong.com>
>
> fixes coccicheck Error:
>
> drivers/staging/greybus/bootrom.c:301:41-45: ERROR:
> fw is NULL but dereferenced.
>
> if procedure goto label directly, ret will be nefative, so the fw is NULL
> and the if(condition) end with dereferenced fw. let's fix it.
No, fw is accessed only for !ret case.
> Signed-off-by: Jian Dong <dongjian@yulong.com>
> ---
> drivers/staging/greybus/bootrom.c | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/staging/greybus/bootrom.c b/drivers/staging/greybus/bootrom.c
> index a8efb86..0439efa 100644
> --- a/drivers/staging/greybus/bootrom.c
> +++ b/drivers/staging/greybus/bootrom.c
> @@ -246,7 +246,7 @@ static int gb_bootrom_get_firmware(struct gb_operation *op)
> struct gb_bootrom_get_firmware_response *firmware_response;
> struct device *dev = &op->connection->bundle->dev;
> unsigned int offset, size;
> - enum next_request_type next_request;
> + enum next_request_type next_request = NEXT_REQ_GET_FIRMWARE;
> int ret = 0;
>
> /* Disable timeouts */
> @@ -298,10 +298,10 @@ static int gb_bootrom_get_firmware(struct gb_operation *op)
>
> queue_work:
> /* Refresh timeout */
> - if (!ret && (offset + size == fw->size))
> - next_request = NEXT_REQ_READY_TO_BOOT;
> - else
> + if (!!ret)
> next_request = NEXT_REQ_GET_FIRMWARE;
> + else if (offset + size == fw->size)
> + next_request = NEXT_REQ_READY_TO_BOOT;
>
> gb_bootrom_set_timeout(bootrom, next_request, NEXT_REQ_TIMEOUT_MS);
The code is fine AFAICT, the coccicheck is buggy as it is detecting a
bug here.
--
viresh
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] staging: greybus: fix fw is NULL but dereferenced
@ 2021-03-25 10:51 ` Viresh Kumar
0 siblings, 0 replies; 18+ messages in thread
From: Viresh Kumar @ 2021-03-25 10:51 UTC (permalink / raw)
To: Jian Dong
Cc: devel, elder, vireshk, johan, linux-kernel, greybus-dev, huyue2,
gregkh, Jian Dong
On 25-03-21, 18:19, Jian Dong wrote:
> From: Jian Dong <dongjian@yulong.com>
>
> fixes coccicheck Error:
>
> drivers/staging/greybus/bootrom.c:301:41-45: ERROR:
> fw is NULL but dereferenced.
>
> if procedure goto label directly, ret will be nefative, so the fw is NULL
> and the if(condition) end with dereferenced fw. let's fix it.
No, fw is accessed only for !ret case.
> Signed-off-by: Jian Dong <dongjian@yulong.com>
> ---
> drivers/staging/greybus/bootrom.c | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/staging/greybus/bootrom.c b/drivers/staging/greybus/bootrom.c
> index a8efb86..0439efa 100644
> --- a/drivers/staging/greybus/bootrom.c
> +++ b/drivers/staging/greybus/bootrom.c
> @@ -246,7 +246,7 @@ static int gb_bootrom_get_firmware(struct gb_operation *op)
> struct gb_bootrom_get_firmware_response *firmware_response;
> struct device *dev = &op->connection->bundle->dev;
> unsigned int offset, size;
> - enum next_request_type next_request;
> + enum next_request_type next_request = NEXT_REQ_GET_FIRMWARE;
> int ret = 0;
>
> /* Disable timeouts */
> @@ -298,10 +298,10 @@ static int gb_bootrom_get_firmware(struct gb_operation *op)
>
> queue_work:
> /* Refresh timeout */
> - if (!ret && (offset + size == fw->size))
> - next_request = NEXT_REQ_READY_TO_BOOT;
> - else
> + if (!!ret)
> next_request = NEXT_REQ_GET_FIRMWARE;
> + else if (offset + size == fw->size)
> + next_request = NEXT_REQ_READY_TO_BOOT;
>
> gb_bootrom_set_timeout(bootrom, next_request, NEXT_REQ_TIMEOUT_MS);
The code is fine AFAICT, the coccicheck is buggy as it is detecting a
bug here.
--
viresh
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
^ permalink raw reply [flat|nested] 18+ messages in thread
end of thread, other threads:[~2021-03-25 11:41 UTC | newest]
Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-01-26 8:31 [PATCH] staging: greybus: fix fw is NULL but dereferenced Saurav Girepunje
2020-01-26 8:31 ` Saurav Girepunje
2020-01-26 11:04 ` Johan Hovold
2020-01-26 11:04 ` Johan Hovold
2020-01-26 18:30 ` Greg KH
2020-01-26 18:30 ` Greg KH
2021-03-25 10:19 Jian Dong
2021-03-25 10:19 ` Jian Dong
2021-03-25 10:29 ` Greg KH
2021-03-25 10:29 ` Greg KH
2021-03-25 11:03 ` Jian Dong
2021-03-25 11:03 ` Jian Dong
2021-03-25 11:32 ` Greg KH
2021-03-25 11:32 ` Greg KH
2021-03-25 10:50 ` Dan Carpenter
2021-03-25 10:50 ` Dan Carpenter
2021-03-25 10:51 ` Viresh Kumar
2021-03-25 10:51 ` Viresh Kumar
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.