[PATCH] ipmi: Fix kernel panic at ipmi_ssif_thread()

[Joeseph Chang]

Signed-off-by: Joeseph Chang 
---
ipmi: Fix kernel panic at ipmi_ssif_thread()

msg_written_handler() may set ssif_info->multi_data to NULL
when using ipmitool to write fru.

Before setting ssif_info->multi_data to NULL, add new local
pointer "data_to_send" and store correct i2c data pointer to
it to fix NULL pointer kernel panic and incorrect ssif_info->multi_pos.

 drivers/char/ipmi/ipmi_ssif.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/char/ipmi/ipmi_ssif.c b/drivers/char/ipmi/ipmi_ssif.c
index cca6e5b..51ba67d 100644
--- a/drivers/char/ipmi/ipmi_ssif.c
+++ b/drivers/char/ipmi/ipmi_ssif.c
@@ -891,6 +891,7 @@ static void msg_written_handler(struct ssif_info 
*ssif_info, int result,
 * for details on the intricacies of this.
 */
int left;
+   unsigned char *data_to_send;
 
ssif_inc_stat(ssif_info, sent_messages_parts);
 
@@ -899,6 +900,7 @@ static void msg_written_handler(struct ssif_info 
*ssif_info, int result,
left = 32;
/* Length byte. */
ssif_info->multi_data[ssif_info->multi_pos] = left;
+   data_to_send = ssif_info->multi_data + ssif_info->multi_pos;
ssif_info->multi_pos += left;
if (left < 32)
/*
@@ -912,7 +914,7 @@ static void msg_written_handler(struct ssif_info 
*ssif_info, int result,
rv = ssif_i2c_send(ssif_info, msg_written_handler,
  I2C_SMBUS_WRITE,
  SSIF_IPMI_MULTI_PART_REQUEST_MIDDLE,
- ssif_info->multi_data + ssif_info->multi_pos,
+ data_to_send,
  I2C_SMBUS_BLOCK_DATA);
if (rv < 0) {
/* request failed, just return the error. */
-- 
1.9.1

[PATCH] ipmi: Fix kernel panic at ipmi_ssif_thread()

[Joeseph Chang]

Signed-off-by: Joeseph Chang 
---
ipmi: Fix kernel panic at ipmi_ssif_thread()

msg_written_handler() may set ssif_info->multi_data to NULL
when using ipmitool to write fru.

Before setting ssif_info->multi_data to NULL, add new local
pointer "data_to_send" and store correct i2c data pointer to
it to fix NULL pointer kernel panic and incorrect ssif_info->multi_pos.

 drivers/char/ipmi/ipmi_ssif.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/char/ipmi/ipmi_ssif.c b/drivers/char/ipmi/ipmi_ssif.c
index cca6e5b..51ba67d 100644
--- a/drivers/char/ipmi/ipmi_ssif.c
+++ b/drivers/char/ipmi/ipmi_ssif.c
@@ -891,6 +891,7 @@ static void msg_written_handler(struct ssif_info 
*ssif_info, int result,
 * for details on the intricacies of this.
 */
int left;
+   unsigned char *data_to_send;
 
ssif_inc_stat(ssif_info, sent_messages_parts);
 
@@ -899,6 +900,7 @@ static void msg_written_handler(struct ssif_info 
*ssif_info, int result,
left = 32;
/* Length byte. */
ssif_info->multi_data[ssif_info->multi_pos] = left;
+   data_to_send = ssif_info->multi_data + ssif_info->multi_pos;
ssif_info->multi_pos += left;
if (left < 32)
/*
@@ -912,7 +914,7 @@ static void msg_written_handler(struct ssif_info 
*ssif_info, int result,
rv = ssif_i2c_send(ssif_info, msg_written_handler,
  I2C_SMBUS_WRITE,
  SSIF_IPMI_MULTI_PART_REQUEST_MIDDLE,
- ssif_info->multi_data + ssif_info->multi_pos,
+ data_to_send,
  I2C_SMBUS_BLOCK_DATA);
if (rv < 0) {
/* request failed, just return the error. */
-- 
1.9.1

Re: [PATCH] ipmi: Fix kernel panic at ipmi_ssif_thread()

[joechang]

Hi Corey,

Thanks your feedback and suggestion. :)
I think you are right. It is better to allocate one new local variable 
to store i2c data and size that will be sent...

I create new patch in following according to your suggestion.
Could you help to review the patch again?

From eec31f51acc7e019fd0dcc512febda86870268d2 Mon Sep 17 00:00:00 2001
From: Joeseph Chang <joech...@codeaurora.org>
Date: Mon, 27 Mar 2017 10:46:42 +0800
Subject: [PATCH] ipmi: Fix kernel panic at ipmi_ssif_thread()

msg_written_handler() may set ssif_info->multi_data to NULL
when using ipmitool to write fru.

Fix kernel panic at ipmi_ssif_thread() and incorrect
ssif_info->multi_pos by adding one local pointer i2c_data to store i2c
data and size to send before calling ssif_i2c_send().

With this change, can avoid concurrent access to ssif_info->multi_pos 
and

ssif_info->multi_data at msg_written_handler.
Because msg_written_handler can be called at any time after 
ssif_i2c_send().

---
 drivers/char/ipmi/ipmi_ssif.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)
 mode change 100644 => 100755 drivers/char/ipmi/ipmi_ssif.c

diff --git a/drivers/char/ipmi/ipmi_ssif.c 
b/drivers/char/ipmi/ipmi_ssif.c

old mode 100644
new mode 100755
index cca6e5b..f36f018
--- a/drivers/char/ipmi/ipmi_ssif.c
+++ b/drivers/char/ipmi/ipmi_ssif.c
@@ -852,6 +852,7 @@ static void msg_written_handler(struct ssif_info 
*ssif_info, int result,

 unsigned char *data, unsigned int len)
 {
 int rv;
+unsigned char *i2c_data;

 /* We are single-threaded here, so no need for a lock. */
 if (result < 0) {
@@ -899,6 +900,7 @@ static void msg_written_handler(struct ssif_info 
*ssif_info, int result,

 left = 32;
 /* Length byte. */
 ssif_info->multi_data[ssif_info->multi_pos] = left;
+i2c_data = ssif_info->multi_data + ssif_info->multi_pos;
 ssif_info->multi_pos += left;
 if (left < 32)
 /*
@@ -912,7 +914,7 @@ static void msg_written_handler(struct ssif_info 
*ssif_info, int result,

 rv = ssif_i2c_send(ssif_info, msg_written_handler,
   I2C_SMBUS_WRITE,
   SSIF_IPMI_MULTI_PART_REQUEST_MIDDLE,
-  ssif_info->multi_data + ssif_info->multi_pos,
+  i2c_data,
   I2C_SMBUS_BLOCK_DATA);
 if (rv < 0) {
 /* request failed, just return the error. */
--
1.9.1


Thank you,
Joseph.

Corey Minyard 於 2017-03-25 10:53 寫到:
This is incorrect.  These values *must* be set before ssif_i2c_send() 
is done.
Once ssif_i2c_send() is called, msg_written_handler can be called at 
any

time after that, including before where you moved the new code.

If I understand this correctly, I think you need to add a variable,
maybe named "bytes_to_write" and do

  bytes_to_write = ssif_info->multi_data + ssif_info->multi_pos;

before the comparison and pass bytes_to_write into
ssif_i2c_send().

Thanks,

-corey

On 03/23/2017 02:07 AM, Joeseph Chang wrote:

From: Joeseph Chang <joech...@codeaurora.org>

msg_written_handler() may set ssif_info->multi_data to NULL
when using ipmitool to write fru.
Change the ssif i2c send data sequence in msg_written_handler()
to fix NULL pointer kernel panic and incorrect ssif_info->multi_pos.

Signed-off-by: Joeseph Chang <joech...@codeaurora.org>
---
  drivers/char/ipmi/ipmi_ssif.c | 20 +++-
  1 file changed, 11 insertions(+), 9 deletions(-)

diff --git a/drivers/char/ipmi/ipmi_ssif.c 
b/drivers/char/ipmi/ipmi_ssif.c

index cca6e5b..39346ee 100644
--- a/drivers/char/ipmi/ipmi_ssif.c
+++ b/drivers/char/ipmi/ipmi_ssif.c
@@ -899,21 +899,13 @@ static void msg_written_handler(struct ssif_info 
*ssif_info, int result,

left = 32;
/* Length byte. */
ssif_info->multi_data[ssif_info->multi_pos] = left;
-   ssif_info->multi_pos += left;
-   if (left < 32)
-   /*
-* Write is finished.  Note that we must end
-* with a write of less than 32 bytes to
-* complete the transaction, even if it is
-* zero bytes.
-*/
-   ssif_info->multi_data = NULL;
rv = ssif_i2c_send(ssif_info, msg_written_handler,
  I2C_SMBUS_WRITE,
  SSIF_IPMI_MULTI_PART_REQUEST_MIDDLE,
  ssif_info->multi_data + ssif_info->multi_pos,
  I2C_SMBUS_BLOCK_DATA);
+
if (rv < 0) {
/* request failed, just return the error. */
ssif_inc_stat(ssif_info, send_errors);
@@ -922,6 +914,16 @@ static void msg_written_handler(struct ssi

Re: [PATCH] ipmi: Fix kernel panic at ipmi_ssif_thread()

[joechang]

Hi Corey,

Thanks your feedback and suggestion. :)
I think you are right. It is better to allocate one new local variable 
to store i2c data and size that will be sent...

I create new patch in following according to your suggestion.
Could you help to review the patch again?

From eec31f51acc7e019fd0dcc512febda86870268d2 Mon Sep 17 00:00:00 2001
From: Joeseph Chang 
Date: Mon, 27 Mar 2017 10:46:42 +0800
Subject: [PATCH] ipmi: Fix kernel panic at ipmi_ssif_thread()

msg_written_handler() may set ssif_info->multi_data to NULL
when using ipmitool to write fru.

Fix kernel panic at ipmi_ssif_thread() and incorrect
ssif_info->multi_pos by adding one local pointer i2c_data to store i2c
data and size to send before calling ssif_i2c_send().

With this change, can avoid concurrent access to ssif_info->multi_pos 
and

ssif_info->multi_data at msg_written_handler.
Because msg_written_handler can be called at any time after 
ssif_i2c_send().

---
 drivers/char/ipmi/ipmi_ssif.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)
 mode change 100644 => 100755 drivers/char/ipmi/ipmi_ssif.c

diff --git a/drivers/char/ipmi/ipmi_ssif.c 
b/drivers/char/ipmi/ipmi_ssif.c

old mode 100644
new mode 100755
index cca6e5b..f36f018
--- a/drivers/char/ipmi/ipmi_ssif.c
+++ b/drivers/char/ipmi/ipmi_ssif.c
@@ -852,6 +852,7 @@ static void msg_written_handler(struct ssif_info 
*ssif_info, int result,

 unsigned char *data, unsigned int len)
 {
 int rv;
+unsigned char *i2c_data;

 /* We are single-threaded here, so no need for a lock. */
 if (result < 0) {
@@ -899,6 +900,7 @@ static void msg_written_handler(struct ssif_info 
*ssif_info, int result,

 left = 32;
 /* Length byte. */
 ssif_info->multi_data[ssif_info->multi_pos] = left;
+i2c_data = ssif_info->multi_data + ssif_info->multi_pos;
 ssif_info->multi_pos += left;
 if (left < 32)
 /*
@@ -912,7 +914,7 @@ static void msg_written_handler(struct ssif_info 
*ssif_info, int result,

 rv = ssif_i2c_send(ssif_info, msg_written_handler,
   I2C_SMBUS_WRITE,
   SSIF_IPMI_MULTI_PART_REQUEST_MIDDLE,
-  ssif_info->multi_data + ssif_info->multi_pos,
+  i2c_data,
   I2C_SMBUS_BLOCK_DATA);
 if (rv < 0) {
 /* request failed, just return the error. */
--
1.9.1


Thank you,
Joseph.

Corey Minyard 於 2017-03-25 10:53 寫到:
This is incorrect.  These values *must* be set before ssif_i2c_send() 
is done.
Once ssif_i2c_send() is called, msg_written_handler can be called at 
any

time after that, including before where you moved the new code.

If I understand this correctly, I think you need to add a variable,
maybe named "bytes_to_write" and do

  bytes_to_write = ssif_info->multi_data + ssif_info->multi_pos;

before the comparison and pass bytes_to_write into
ssif_i2c_send().

Thanks,

-corey

On 03/23/2017 02:07 AM, Joeseph Chang wrote:

From: Joeseph Chang 

msg_written_handler() may set ssif_info->multi_data to NULL
when using ipmitool to write fru.
Change the ssif i2c send data sequence in msg_written_handler()
to fix NULL pointer kernel panic and incorrect ssif_info->multi_pos.

Signed-off-by: Joeseph Chang 
---
  drivers/char/ipmi/ipmi_ssif.c | 20 +++-
  1 file changed, 11 insertions(+), 9 deletions(-)

diff --git a/drivers/char/ipmi/ipmi_ssif.c 
b/drivers/char/ipmi/ipmi_ssif.c

index cca6e5b..39346ee 100644
--- a/drivers/char/ipmi/ipmi_ssif.c
+++ b/drivers/char/ipmi/ipmi_ssif.c
@@ -899,21 +899,13 @@ static void msg_written_handler(struct ssif_info 
*ssif_info, int result,

left = 32;
/* Length byte. */
ssif_info->multi_data[ssif_info->multi_pos] = left;
-   ssif_info->multi_pos += left;
-   if (left < 32)
-   /*
-* Write is finished.  Note that we must end
-* with a write of less than 32 bytes to
-* complete the transaction, even if it is
-* zero bytes.
-*/
-   ssif_info->multi_data = NULL;
rv = ssif_i2c_send(ssif_info, msg_written_handler,
  I2C_SMBUS_WRITE,
  SSIF_IPMI_MULTI_PART_REQUEST_MIDDLE,
  ssif_info->multi_data + ssif_info->multi_pos,
  I2C_SMBUS_BLOCK_DATA);
+
if (rv < 0) {
/* request failed, just return the error. */
ssif_inc_stat(ssif_info, send_errors);
@@ -922,6 +914,16 @@ static void msg_written_handler(struct ssif_info 
*ssif_info, int result,

pr_info("Error fr

RE: [PATCH] ipmi: Fix kernel panic at ipmi_ssif_thread()

[Joseph Chang]

Hi Corey,

Thanks your feedback and suggestion. :)
I think you are right. It is better to allocate one new local variable to store 
i2c data and size to be send...
I create new patch in following according to your suggestion.
Could you help to review the patch again?

>From eec31f51acc7e019fd0dcc512febda86870268d2 Mon Sep 17 00:00:00 2001
From: Joeseph Chang <joech...@codeaurora.org>
Date: Mon, 27 Mar 2017 10:46:42 +0800
Subject: [PATCH] ipmi: Fix kernel panic at ipmi_ssif_thread()

msg_written_handler() may set ssif_info->multi_data to NULL
when using ipmitool to write fru.

Fix kernel panic at ipmi_ssif_thread() and incorrect
ssif_info->multi_pos by adding one local pointer i2c_data to store i2c
data and size to send before calling ssif_i2c_send().

With this change, can avoid concurrent access to ssif_info->multi_pos and
ssif_info->multi_data at msg_written_handler.
Because msg_written_handler can be called at any time after ssif_i2c_send().
---
 drivers/char/ipmi/ipmi_ssif.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)
 mode change 100644 => 100755 drivers/char/ipmi/ipmi_ssif.c

diff --git a/drivers/char/ipmi/ipmi_ssif.c b/drivers/char/ipmi/ipmi_ssif.c
old mode 100644
new mode 100755
index cca6e5b..f36f018
--- a/drivers/char/ipmi/ipmi_ssif.c
+++ b/drivers/char/ipmi/ipmi_ssif.c
@@ -852,6 +852,7 @@ static void msg_written_handler(struct ssif_info 
*ssif_info, int result,
unsigned char *data, unsigned int len)
 {
int rv;
+   unsigned char *i2c_data;
 
/* We are single-threaded here, so no need for a lock. */
if (result < 0) {
@@ -899,6 +900,7 @@ static void msg_written_handler(struct ssif_info 
*ssif_info, int result,
left = 32;
/* Length byte. */
ssif_info->multi_data[ssif_info->multi_pos] = left;
+   i2c_data = ssif_info->multi_data + ssif_info->multi_pos;
ssif_info->multi_pos += left;
if (left < 32)
/*
@@ -912,7 +914,7 @@ static void msg_written_handler(struct ssif_info 
*ssif_info, int result,
rv = ssif_i2c_send(ssif_info, msg_written_handler,
  I2C_SMBUS_WRITE,
  SSIF_IPMI_MULTI_PART_REQUEST_MIDDLE,
- ssif_info->multi_data + ssif_info->multi_pos,
+ i2c_data,
  I2C_SMBUS_BLOCK_DATA);
if (rv < 0) {
/* request failed, just return the error. */
-- 
1.9.1


Thank you,
Joseph.
-Original Message-
From: Corey Minyard [mailto:tcminy...@gmail.com] On Behalf Of Corey Minyard
Sent: Saturday, March 25, 2017 10:53 AM
To: Joseph Chang <joech...@qti.qualcomm.com>; 
openipmi-develo...@lists.sourceforge.net; linux-kernel@vger.kernel.org
Cc: Jiandi An <anjia...@qti.qualcomm.com>; Joeseph Chang 
<joech...@codeaurora.org>
Subject: Re: [PATCH] ipmi: Fix kernel panic at ipmi_ssif_thread()

This is incorrect.  These values *must* be set before ssif_i2c_send() is done.
Once ssif_i2c_send() is called, msg_written_handler can be called at any time 
after that, including before where you moved the new code.

If I understand this correctly, I think you need to add a variable, maybe named 
"bytes_to_write" and do

   bytes_to_write = ssif_info->multi_data + ssif_info->multi_pos;

before the comparison and pass bytes_to_write into ssif_i2c_send().

Thanks,

-corey

On 03/23/2017 02:07 AM, Joeseph Chang wrote:
> From: Joeseph Chang <joech...@codeaurora.org>
>
> msg_written_handler() may set ssif_info->multi_data to NULL when using 
> ipmitool to write fru.
> Change the ssif i2c send data sequence in msg_written_handler() to fix 
> NULL pointer kernel panic and incorrect ssif_info->multi_pos.
>
> Signed-off-by: Joeseph Chang <joech...@codeaurora.org>
> ---
>   drivers/char/ipmi/ipmi_ssif.c | 20 +++-
>   1 file changed, 11 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/char/ipmi/ipmi_ssif.c 
> b/drivers/char/ipmi/ipmi_ssif.c index cca6e5b..39346ee 100644
> --- a/drivers/char/ipmi/ipmi_ssif.c
> +++ b/drivers/char/ipmi/ipmi_ssif.c
> @@ -899,21 +899,13 @@ static void msg_written_handler(struct ssif_info 
> *ssif_info, int result,
>   left = 32;
>   /* Length byte. */
>   ssif_info->multi_data[ssif_info->multi_pos] = left;
> - ssif_info->multi_pos += left;
> - if (left < 32)
> - /*
> -  * Write is finished.  Note that we must end
> -  * with a write of less than 32 bytes to
> -  * complete the transaction, even i

RE: [PATCH] ipmi: Fix kernel panic at ipmi_ssif_thread()

[Joseph Chang]

Hi Corey,

Thanks your feedback and suggestion. :)
I think you are right. It is better to allocate one new local variable to store 
i2c data and size to be send...
I create new patch in following according to your suggestion.
Could you help to review the patch again?

>From eec31f51acc7e019fd0dcc512febda86870268d2 Mon Sep 17 00:00:00 2001
From: Joeseph Chang 
Date: Mon, 27 Mar 2017 10:46:42 +0800
Subject: [PATCH] ipmi: Fix kernel panic at ipmi_ssif_thread()

msg_written_handler() may set ssif_info->multi_data to NULL
when using ipmitool to write fru.

Fix kernel panic at ipmi_ssif_thread() and incorrect
ssif_info->multi_pos by adding one local pointer i2c_data to store i2c
data and size to send before calling ssif_i2c_send().

With this change, can avoid concurrent access to ssif_info->multi_pos and
ssif_info->multi_data at msg_written_handler.
Because msg_written_handler can be called at any time after ssif_i2c_send().
---
 drivers/char/ipmi/ipmi_ssif.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)
 mode change 100644 => 100755 drivers/char/ipmi/ipmi_ssif.c

diff --git a/drivers/char/ipmi/ipmi_ssif.c b/drivers/char/ipmi/ipmi_ssif.c
old mode 100644
new mode 100755
index cca6e5b..f36f018
--- a/drivers/char/ipmi/ipmi_ssif.c
+++ b/drivers/char/ipmi/ipmi_ssif.c
@@ -852,6 +852,7 @@ static void msg_written_handler(struct ssif_info 
*ssif_info, int result,
unsigned char *data, unsigned int len)
 {
int rv;
+   unsigned char *i2c_data;
 
/* We are single-threaded here, so no need for a lock. */
if (result < 0) {
@@ -899,6 +900,7 @@ static void msg_written_handler(struct ssif_info 
*ssif_info, int result,
left = 32;
/* Length byte. */
ssif_info->multi_data[ssif_info->multi_pos] = left;
+   i2c_data = ssif_info->multi_data + ssif_info->multi_pos;
ssif_info->multi_pos += left;
if (left < 32)
/*
@@ -912,7 +914,7 @@ static void msg_written_handler(struct ssif_info 
*ssif_info, int result,
rv = ssif_i2c_send(ssif_info, msg_written_handler,
  I2C_SMBUS_WRITE,
  SSIF_IPMI_MULTI_PART_REQUEST_MIDDLE,
- ssif_info->multi_data + ssif_info->multi_pos,
+ i2c_data,
  I2C_SMBUS_BLOCK_DATA);
if (rv < 0) {
/* request failed, just return the error. */
-- 
1.9.1


Thank you,
Joseph.
-Original Message-
From: Corey Minyard [mailto:tcminy...@gmail.com] On Behalf Of Corey Minyard
Sent: Saturday, March 25, 2017 10:53 AM
To: Joseph Chang ; 
openipmi-develo...@lists.sourceforge.net; linux-kernel@vger.kernel.org
Cc: Jiandi An ; Joeseph Chang 

Subject: Re: [PATCH] ipmi: Fix kernel panic at ipmi_ssif_thread()

This is incorrect.  These values *must* be set before ssif_i2c_send() is done.
Once ssif_i2c_send() is called, msg_written_handler can be called at any time 
after that, including before where you moved the new code.

If I understand this correctly, I think you need to add a variable, maybe named 
"bytes_to_write" and do

   bytes_to_write = ssif_info->multi_data + ssif_info->multi_pos;

before the comparison and pass bytes_to_write into ssif_i2c_send().

Thanks,

-corey

On 03/23/2017 02:07 AM, Joeseph Chang wrote:
> From: Joeseph Chang 
>
> msg_written_handler() may set ssif_info->multi_data to NULL when using 
> ipmitool to write fru.
> Change the ssif i2c send data sequence in msg_written_handler() to fix 
> NULL pointer kernel panic and incorrect ssif_info->multi_pos.
>
> Signed-off-by: Joeseph Chang 
> ---
>   drivers/char/ipmi/ipmi_ssif.c | 20 +++-
>   1 file changed, 11 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/char/ipmi/ipmi_ssif.c 
> b/drivers/char/ipmi/ipmi_ssif.c index cca6e5b..39346ee 100644
> --- a/drivers/char/ipmi/ipmi_ssif.c
> +++ b/drivers/char/ipmi/ipmi_ssif.c
> @@ -899,21 +899,13 @@ static void msg_written_handler(struct ssif_info 
> *ssif_info, int result,
>   left = 32;
>   /* Length byte. */
>   ssif_info->multi_data[ssif_info->multi_pos] = left;
> - ssif_info->multi_pos += left;
> - if (left < 32)
> - /*
> -  * Write is finished.  Note that we must end
> -  * with a write of less than 32 bytes to
> -  * complete the transaction, even if it is
> -  * zero bytes.
> -  */
> - ssif_info->multi_data = NULL;
>   
>   rv = ssif_i2c_send(ssif_info, msg_written_handler,
>   

Re: [PATCH] ipmi: Fix kernel panic at ipmi_ssif_thread()

[Corey Minyard]

This is incorrect.  These values *must* be set before ssif_i2c_send() is 
done.

Once ssif_i2c_send() is called, msg_written_handler can be called at any
time after that, including before where you moved the new code.

If I understand this correctly, I think you need to add a variable,
maybe named "bytes_to_write" and do

  bytes_to_write = ssif_info->multi_data + ssif_info->multi_pos;

before the comparison and pass bytes_to_write into
ssif_i2c_send().

Thanks,

-corey

On 03/23/2017 02:07 AM, Joeseph Chang wrote:

From: Joeseph Chang 

msg_written_handler() may set ssif_info->multi_data to NULL
when using ipmitool to write fru.
Change the ssif i2c send data sequence in msg_written_handler()
to fix NULL pointer kernel panic and incorrect ssif_info->multi_pos.

Signed-off-by: Joeseph Chang 
---
  drivers/char/ipmi/ipmi_ssif.c | 20 +++-
  1 file changed, 11 insertions(+), 9 deletions(-)

diff --git a/drivers/char/ipmi/ipmi_ssif.c b/drivers/char/ipmi/ipmi_ssif.c
index cca6e5b..39346ee 100644
--- a/drivers/char/ipmi/ipmi_ssif.c
+++ b/drivers/char/ipmi/ipmi_ssif.c
@@ -899,21 +899,13 @@ static void msg_written_handler(struct ssif_info 
*ssif_info, int result,
left = 32;
/* Length byte. */
ssif_info->multi_data[ssif_info->multi_pos] = left;
-   ssif_info->multi_pos += left;
-   if (left < 32)
-   /*
-* Write is finished.  Note that we must end
-* with a write of less than 32 bytes to
-* complete the transaction, even if it is
-* zero bytes.
-*/
-   ssif_info->multi_data = NULL;
  
  		rv = ssif_i2c_send(ssif_info, msg_written_handler,

  I2C_SMBUS_WRITE,
  SSIF_IPMI_MULTI_PART_REQUEST_MIDDLE,
  ssif_info->multi_data + ssif_info->multi_pos,
  I2C_SMBUS_BLOCK_DATA);
+
if (rv < 0) {
/* request failed, just return the error. */
ssif_inc_stat(ssif_info, send_errors);
@@ -922,6 +914,16 @@ static void msg_written_handler(struct ssif_info 
*ssif_info, int result,
pr_info("Error from i2c_non_blocking_op(3)\n");
msg_done_handler(ssif_info, -EIO, NULL, 0);
}
+
+   ssif_info->multi_pos += left;
+   if (left < 32)
+   /*
+* Write is finished.  Note that we must end
+* with a write of less than 32 bytes to
+* complete the transaction, even if it is
+* zero bytes.
+*/
+   ssif_info->multi_data = NULL;
} else {
/* Ready to request the result. */
unsigned long oflags, *flags;

Re: [PATCH] ipmi: Fix kernel panic at ipmi_ssif_thread()

[Corey Minyard]

This is incorrect.  These values *must* be set before ssif_i2c_send() is 
done.

Once ssif_i2c_send() is called, msg_written_handler can be called at any
time after that, including before where you moved the new code.

If I understand this correctly, I think you need to add a variable,
maybe named "bytes_to_write" and do

  bytes_to_write = ssif_info->multi_data + ssif_info->multi_pos;

before the comparison and pass bytes_to_write into
ssif_i2c_send().

Thanks,

-corey

On 03/23/2017 02:07 AM, Joeseph Chang wrote:

From: Joeseph Chang 

msg_written_handler() may set ssif_info->multi_data to NULL
when using ipmitool to write fru.
Change the ssif i2c send data sequence in msg_written_handler()
to fix NULL pointer kernel panic and incorrect ssif_info->multi_pos.

Signed-off-by: Joeseph Chang 
---
  drivers/char/ipmi/ipmi_ssif.c | 20 +++-
  1 file changed, 11 insertions(+), 9 deletions(-)

diff --git a/drivers/char/ipmi/ipmi_ssif.c b/drivers/char/ipmi/ipmi_ssif.c
index cca6e5b..39346ee 100644
--- a/drivers/char/ipmi/ipmi_ssif.c
+++ b/drivers/char/ipmi/ipmi_ssif.c
@@ -899,21 +899,13 @@ static void msg_written_handler(struct ssif_info 
*ssif_info, int result,
left = 32;
/* Length byte. */
ssif_info->multi_data[ssif_info->multi_pos] = left;
-   ssif_info->multi_pos += left;
-   if (left < 32)
-   /*
-* Write is finished.  Note that we must end
-* with a write of less than 32 bytes to
-* complete the transaction, even if it is
-* zero bytes.
-*/
-   ssif_info->multi_data = NULL;
  
  		rv = ssif_i2c_send(ssif_info, msg_written_handler,

  I2C_SMBUS_WRITE,
  SSIF_IPMI_MULTI_PART_REQUEST_MIDDLE,
  ssif_info->multi_data + ssif_info->multi_pos,
  I2C_SMBUS_BLOCK_DATA);
+
if (rv < 0) {
/* request failed, just return the error. */
ssif_inc_stat(ssif_info, send_errors);
@@ -922,6 +914,16 @@ static void msg_written_handler(struct ssif_info 
*ssif_info, int result,
pr_info("Error from i2c_non_blocking_op(3)\n");
msg_done_handler(ssif_info, -EIO, NULL, 0);
}
+
+   ssif_info->multi_pos += left;
+   if (left < 32)
+   /*
+* Write is finished.  Note that we must end
+* with a write of less than 32 bytes to
+* complete the transaction, even if it is
+* zero bytes.
+*/
+   ssif_info->multi_data = NULL;
} else {
/* Ready to request the result. */
unsigned long oflags, *flags;

[PATCH] ipmi: Fix kernel panic at ipmi_ssif_thread()

[Joeseph Chang]

From: Joeseph Chang 

msg_written_handler() may set ssif_info->multi_data to NULL
when using ipmitool to write fru.
Change the ssif i2c send data sequence in msg_written_handler()
to fix NULL pointer kernel panic and incorrect ssif_info->multi_pos.

Signed-off-by: Joeseph Chang 
---
 drivers/char/ipmi/ipmi_ssif.c | 20 +++-
 1 file changed, 11 insertions(+), 9 deletions(-)

diff --git a/drivers/char/ipmi/ipmi_ssif.c b/drivers/char/ipmi/ipmi_ssif.c
index cca6e5b..39346ee 100644
--- a/drivers/char/ipmi/ipmi_ssif.c
+++ b/drivers/char/ipmi/ipmi_ssif.c
@@ -899,21 +899,13 @@ static void msg_written_handler(struct ssif_info 
*ssif_info, int result,
left = 32;
/* Length byte. */
ssif_info->multi_data[ssif_info->multi_pos] = left;
-   ssif_info->multi_pos += left;
-   if (left < 32)
-   /*
-* Write is finished.  Note that we must end
-* with a write of less than 32 bytes to
-* complete the transaction, even if it is
-* zero bytes.
-*/
-   ssif_info->multi_data = NULL;
 
rv = ssif_i2c_send(ssif_info, msg_written_handler,
  I2C_SMBUS_WRITE,
  SSIF_IPMI_MULTI_PART_REQUEST_MIDDLE,
  ssif_info->multi_data + ssif_info->multi_pos,
  I2C_SMBUS_BLOCK_DATA);
+
if (rv < 0) {
/* request failed, just return the error. */
ssif_inc_stat(ssif_info, send_errors);
@@ -922,6 +914,16 @@ static void msg_written_handler(struct ssif_info 
*ssif_info, int result,
pr_info("Error from i2c_non_blocking_op(3)\n");
msg_done_handler(ssif_info, -EIO, NULL, 0);
}
+
+   ssif_info->multi_pos += left;
+   if (left < 32)
+   /*
+* Write is finished.  Note that we must end
+* with a write of less than 32 bytes to
+* complete the transaction, even if it is
+* zero bytes.
+*/
+   ssif_info->multi_data = NULL;
} else {
/* Ready to request the result. */
unsigned long oflags, *flags;
-- 
1.9.1

[PATCH] ipmi: Fix kernel panic at ipmi_ssif_thread()

[Joeseph Chang]

From: Joeseph Chang 

msg_written_handler() may set ssif_info->multi_data to NULL
when using ipmitool to write fru.
Change the ssif i2c send data sequence in msg_written_handler()
to fix NULL pointer kernel panic and incorrect ssif_info->multi_pos.

Signed-off-by: Joeseph Chang 
---
 drivers/char/ipmi/ipmi_ssif.c | 20 +++-
 1 file changed, 11 insertions(+), 9 deletions(-)

diff --git a/drivers/char/ipmi/ipmi_ssif.c b/drivers/char/ipmi/ipmi_ssif.c
index cca6e5b..39346ee 100644
--- a/drivers/char/ipmi/ipmi_ssif.c
+++ b/drivers/char/ipmi/ipmi_ssif.c
@@ -899,21 +899,13 @@ static void msg_written_handler(struct ssif_info 
*ssif_info, int result,
left = 32;
/* Length byte. */
ssif_info->multi_data[ssif_info->multi_pos] = left;
-   ssif_info->multi_pos += left;
-   if (left < 32)
-   /*
-* Write is finished.  Note that we must end
-* with a write of less than 32 bytes to
-* complete the transaction, even if it is
-* zero bytes.
-*/
-   ssif_info->multi_data = NULL;
 
rv = ssif_i2c_send(ssif_info, msg_written_handler,
  I2C_SMBUS_WRITE,
  SSIF_IPMI_MULTI_PART_REQUEST_MIDDLE,
  ssif_info->multi_data + ssif_info->multi_pos,
  I2C_SMBUS_BLOCK_DATA);
+
if (rv < 0) {
/* request failed, just return the error. */
ssif_inc_stat(ssif_info, send_errors);
@@ -922,6 +914,16 @@ static void msg_written_handler(struct ssif_info 
*ssif_info, int result,
pr_info("Error from i2c_non_blocking_op(3)\n");
msg_done_handler(ssif_info, -EIO, NULL, 0);
}
+
+   ssif_info->multi_pos += left;
+   if (left < 32)
+   /*
+* Write is finished.  Note that we must end
+* with a write of less than 32 bytes to
+* complete the transaction, even if it is
+* zero bytes.
+*/
+   ssif_info->multi_data = NULL;
} else {
/* Ready to request the result. */
unsigned long oflags, *flags;
-- 
1.9.1