* [PATCH] ipmi/powernv: Fix potential invalid pointer dereference @ 2015-07-16 11:16 Neelesh Gupta 2015-07-16 15:01 ` Corey Minyard 0 siblings, 1 reply; 5+ messages in thread From: Neelesh Gupta @ 2015-07-16 11:16 UTC (permalink / raw) To: alistair, linuxppc-dev, jk, minyard If the OPAL call to receive the ipmi message fails, then we free up the smi message and return. But, the driver still holds the reference to old smi message in the 'cur_msg' which can potentially be accessed later and freed again leading to kernel oops. To fix it up, The kernel driver should reset the 'cur_msg' and send reply to the user in addition to freeing the message. Signed-off-by: Neelesh Gupta <neelegup@linux.vnet.ibm.com> --- drivers/char/ipmi/ipmi_powernv.c | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/drivers/char/ipmi/ipmi_powernv.c b/drivers/char/ipmi/ipmi_powernv.c index 9b409c0..637486d 100644 --- a/drivers/char/ipmi/ipmi_powernv.c +++ b/drivers/char/ipmi/ipmi_powernv.c @@ -143,9 +143,16 @@ static int ipmi_powernv_recv(struct ipmi_smi_powernv *smi) pr_devel("%s: -> %d (size %lld)\n", __func__, rc, rc == 0 ? size : 0); if (rc) { - spin_unlock_irqrestore(&smi->msg_lock, flags); - ipmi_free_smi_msg(msg); - return 0; + /* If came via the poll, and response was not yet ready */ + if (rc == OPAL_EMPTY) { + spin_unlock_irqrestore(&smi->msg_lock, flags); + return 0; + } else { + smi->cur_msg = NULL; + spin_unlock_irqrestore(&smi->msg_lock, flags); + send_error_reply(smi, msg, IPMI_ERR_UNSPECIFIED); + return 0; + } } if (size < sizeof(*opal_msg)) { ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] ipmi/powernv: Fix potential invalid pointer dereference 2015-07-16 11:16 [PATCH] ipmi/powernv: Fix potential invalid pointer dereference Neelesh Gupta @ 2015-07-16 15:01 ` Corey Minyard 2015-07-17 8:42 ` Neelesh Gupta 0 siblings, 1 reply; 5+ messages in thread From: Corey Minyard @ 2015-07-16 15:01 UTC (permalink / raw) To: Neelesh Gupta, alistair, linuxppc-dev, jk Ok, this looks fine. A couple of question... Do I need to send this upstream right now? How well has this been tested? Do you want this backported to 4.0 stable? -corey On 07/16/2015 06:16 AM, Neelesh Gupta wrote: > If the OPAL call to receive the ipmi message fails, then we free up the > smi message and return. But, the driver still holds the reference to > old smi message in the 'cur_msg' which can potentially be accessed later > and freed again leading to kernel oops. To fix it up, > > The kernel driver should reset the 'cur_msg' and send reply to the user > in addition to freeing the message. > > Signed-off-by: Neelesh Gupta <neelegup@linux.vnet.ibm.com> > --- > drivers/char/ipmi/ipmi_powernv.c | 13 ++++++++++--- > 1 file changed, 10 insertions(+), 3 deletions(-) > > diff --git a/drivers/char/ipmi/ipmi_powernv.c b/drivers/char/ipmi/ipmi_powernv.c > index 9b409c0..637486d 100644 > --- a/drivers/char/ipmi/ipmi_powernv.c > +++ b/drivers/char/ipmi/ipmi_powernv.c > @@ -143,9 +143,16 @@ static int ipmi_powernv_recv(struct ipmi_smi_powernv *smi) > pr_devel("%s: -> %d (size %lld)\n", __func__, > rc, rc == 0 ? size : 0); > if (rc) { > - spin_unlock_irqrestore(&smi->msg_lock, flags); > - ipmi_free_smi_msg(msg); > - return 0; > + /* If came via the poll, and response was not yet ready */ > + if (rc == OPAL_EMPTY) { > + spin_unlock_irqrestore(&smi->msg_lock, flags); > + return 0; > + } else { > + smi->cur_msg = NULL; > + spin_unlock_irqrestore(&smi->msg_lock, flags); > + send_error_reply(smi, msg, IPMI_ERR_UNSPECIFIED); > + return 0; > + } > } > > if (size < sizeof(*opal_msg)) { > ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] ipmi/powernv: Fix potential invalid pointer dereference 2015-07-16 15:01 ` Corey Minyard @ 2015-07-17 8:42 ` Neelesh Gupta [not found] ` <55B7342F.8080703@linux.vnet.ibm.com> 0 siblings, 1 reply; 5+ messages in thread From: Neelesh Gupta @ 2015-07-17 8:42 UTC (permalink / raw) To: minyard, alistair, linuxppc-dev, jk [-- Attachment #1: Type: text/plain, Size: 2206 bytes --] Hi Corey, On 07/16/2015 08:31 PM, Corey Minyard wrote: > Ok, this looks fine. A couple of question... > > Do I need to send this upstream right now? How well has this been tested? I would want either Jeremy or Alistair to review this patch before you send this upstream. There is also firmware piece http://patchwork.ozlabs.org/patch/496645/ awaiting review. In the testing front, I manually made the opal_ipmi_recv() function to fail for testing the error path and see if the driver recovers from it and subsequent ipmi commands work all good. > > Do you want this backported to 4.0 stable? Yes, I want this to be be backported to 4.0 stable. Thanks, Neelesh. > > -corey > > On 07/16/2015 06:16 AM, Neelesh Gupta wrote: >> If the OPAL call to receive the ipmi message fails, then we free up the >> smi message and return. But, the driver still holds the reference to >> old smi message in the 'cur_msg' which can potentially be accessed later >> and freed again leading to kernel oops. To fix it up, >> >> The kernel driver should reset the 'cur_msg' and send reply to the user >> in addition to freeing the message. >> >> Signed-off-by: Neelesh Gupta <neelegup@linux.vnet.ibm.com> >> --- >> drivers/char/ipmi/ipmi_powernv.c | 13 ++++++++++--- >> 1 file changed, 10 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/char/ipmi/ipmi_powernv.c b/drivers/char/ipmi/ipmi_powernv.c >> index 9b409c0..637486d 100644 >> --- a/drivers/char/ipmi/ipmi_powernv.c >> +++ b/drivers/char/ipmi/ipmi_powernv.c >> @@ -143,9 +143,16 @@ static int ipmi_powernv_recv(struct ipmi_smi_powernv *smi) >> pr_devel("%s: -> %d (size %lld)\n", __func__, >> rc, rc == 0 ? size : 0); >> if (rc) { >> - spin_unlock_irqrestore(&smi->msg_lock, flags); >> - ipmi_free_smi_msg(msg); >> - return 0; >> + /* If came via the poll, and response was not yet ready */ >> + if (rc == OPAL_EMPTY) { >> + spin_unlock_irqrestore(&smi->msg_lock, flags); >> + return 0; >> + } else { >> + smi->cur_msg = NULL; >> + spin_unlock_irqrestore(&smi->msg_lock, flags); >> + send_error_reply(smi, msg, IPMI_ERR_UNSPECIFIED); >> + return 0; >> + } >> } >> >> if (size < sizeof(*opal_msg)) { >> [-- Attachment #2: Type: text/html, Size: 3147 bytes --] ^ permalink raw reply [flat|nested] 5+ messages in thread
[parent not found: <55B7342F.8080703@linux.vnet.ibm.com>]
* Re: [PATCH] ipmi/powernv: Fix potential invalid pointer dereference [not found] ` <55B7342F.8080703@linux.vnet.ibm.com> @ 2015-07-28 17:51 ` Alistair Popple 2015-07-29 6:05 ` Neelesh Gupta 0 siblings, 1 reply; 5+ messages in thread From: Alistair Popple @ 2015-07-28 17:51 UTC (permalink / raw) To: Neelesh Gupta; +Cc: jk, linuxppc-dev, minyard Hi Neelesh, This fix looks reasonable to me, although Jeremy would be the best person to comment if he has time. I wonder why we bother polling at all given that our event interface should call opal_ipmi_recv() whenever a message is ready? Also the firmware fix you refer to and this fix are independent of each other so there's no ordering issues there. Reviewed-By: Alistair Popple <alistair@popple.id.au> On Tue, 28 Jul 2015 13:20:07 Neelesh Gupta wrote: > > On 07/17/2015 02:12 PM, Neelesh Gupta wrote: > > Hi Corey, > > > > On 07/16/2015 08:31 PM, Corey Minyard wrote: > >> Ok, this looks fine. A couple of question... > >> > >> Do I need to send this upstream right now? How well has this been tested? > > > > I would want either Jeremy or Alistair to review this patch before you > > send this > > upstream. There is also firmware piece > > http://patchwork.ozlabs.org/patch/496645/ > > awaiting review. > > > > In the testing front, I manually made the opal_ipmi_recv() function to > > fail for testing > > the error path and see if the driver recovers from it and subsequent > > ipmi commands > > work all good. > > Hi Jeremy/Alistair, > > Could you please review it and the corresponding skiboot patch... > > Thanks, > Neelesh. > > > > >> Do you want this backported to 4.0 stable? > > > > Yes, I want this to be be backported to 4.0 stable. > > > > Thanks, > > Neelesh. > > > >> -corey > >> > >> On 07/16/2015 06:16 AM, Neelesh Gupta wrote: > >>> If the OPAL call to receive the ipmi message fails, then we free up the > >>> smi message and return. But, the driver still holds the reference to > >>> old smi message in the 'cur_msg' which can potentially be accessed later > >>> and freed again leading to kernel oops. To fix it up, > >>> > >>> The kernel driver should reset the 'cur_msg' and send reply to the user > >>> in addition to freeing the message. > >>> > >>> Signed-off-by: Neelesh Gupta<neelegup@linux.vnet.ibm.com> > >>> --- > >>> drivers/char/ipmi/ipmi_powernv.c | 13 ++++++++++--- > >>> 1 file changed, 10 insertions(+), 3 deletions(-) > >>> > >>> diff --git a/drivers/char/ipmi/ipmi_powernv.c b/drivers/char/ipmi/ipmi_powernv.c > >>> index 9b409c0..637486d 100644 > >>> --- a/drivers/char/ipmi/ipmi_powernv.c > >>> +++ b/drivers/char/ipmi/ipmi_powernv.c > >>> @@ -143,9 +143,16 @@ static int ipmi_powernv_recv(struct ipmi_smi_powernv *smi) > >>> pr_devel("%s: -> %d (size %lld)\n", __func__, > >>> rc, rc == 0 ? size : 0); > >>> if (rc) { > >>> - spin_unlock_irqrestore(&smi->msg_lock, flags); > >>> - ipmi_free_smi_msg(msg); > >>> - return 0; > >>> + /* If came via the poll, and response was not yet ready */ > >>> + if (rc == OPAL_EMPTY) { > >>> + spin_unlock_irqrestore(&smi->msg_lock, flags); > >>> + return 0; > >>> + } else { > >>> + smi->cur_msg = NULL; > >>> + spin_unlock_irqrestore(&smi->msg_lock, flags); > >>> + send_error_reply(smi, msg, IPMI_ERR_UNSPECIFIED); > >>> + return 0; > >>> + } > >>> } > >>> > >>> if (size < sizeof(*opal_msg)) { > >>> > > > > > > > > _______________________________________________ > > Linuxppc-dev mailing list > > Linuxppc-dev@lists.ozlabs.org > > https://lists.ozlabs.org/listinfo/linuxppc-dev > ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] ipmi/powernv: Fix potential invalid pointer dereference 2015-07-28 17:51 ` Alistair Popple @ 2015-07-29 6:05 ` Neelesh Gupta 0 siblings, 0 replies; 5+ messages in thread From: Neelesh Gupta @ 2015-07-29 6:05 UTC (permalink / raw) To: Alistair Popple; +Cc: jk, linuxppc-dev, minyard [-- Attachment #1: Type: text/plain, Size: 3737 bytes --] Hi Alistair, Thanks for the review. On 07/28/2015 11:21 PM, Alistair Popple wrote: > Hi Neelesh, > > This fix looks reasonable to me, although Jeremy would be the best person to > comment if he has time. I wonder why we bother polling at all given that our > event interface should call opal_ipmi_recv() whenever a message is ready? Agree. I thought about it and didn't find any reason to have it as we have event mechanism.. but didn't think of changing as it is not causing any issue.. > > Also the firmware fix you refer to and this fix are independent of each other > so there's no ordering issues there. Correct. Though, there is no relation, but I figured out the skiboot issue after this change.. yes, they are independent. Please find time to review the skiboot patch. Corey, Please queue this patch for upstream if you Ok with it. Thanks, Neelesh. > > Reviewed-By: Alistair Popple <alistair@popple.id.au> > > On Tue, 28 Jul 2015 13:20:07 Neelesh Gupta wrote: >> On 07/17/2015 02:12 PM, Neelesh Gupta wrote: >>> Hi Corey, >>> >>> On 07/16/2015 08:31 PM, Corey Minyard wrote: >>>> Ok, this looks fine. A couple of question... >>>> >>>> Do I need to send this upstream right now? How well has this been > tested? >>> I would want either Jeremy or Alistair to review this patch before you >>> send this >>> upstream. There is also firmware piece >>> http://patchwork.ozlabs.org/patch/496645/ >>> awaiting review. >>> >>> In the testing front, I manually made the opal_ipmi_recv() function to >>> fail for testing >>> the error path and see if the driver recovers from it and subsequent >>> ipmi commands >>> work all good. >> Hi Jeremy/Alistair, >> >> Could you please review it and the corresponding skiboot patch... >> >> Thanks, >> Neelesh. >> >>>> Do you want this backported to 4.0 stable? >>> Yes, I want this to be be backported to 4.0 stable. >>> >>> Thanks, >>> Neelesh. >>> >>>> -corey >>>> >>>> On 07/16/2015 06:16 AM, Neelesh Gupta wrote: >>>>> If the OPAL call to receive the ipmi message fails, then we free up the >>>>> smi message and return. But, the driver still holds the reference to >>>>> old smi message in the 'cur_msg' which can potentially be accessed later >>>>> and freed again leading to kernel oops. To fix it up, >>>>> >>>>> The kernel driver should reset the 'cur_msg' and send reply to the user >>>>> in addition to freeing the message. >>>>> >>>>> Signed-off-by: Neelesh Gupta<neelegup@linux.vnet.ibm.com> >>>>> --- >>>>> drivers/char/ipmi/ipmi_powernv.c | 13 ++++++++++--- >>>>> 1 file changed, 10 insertions(+), 3 deletions(-) >>>>> >>>>> diff --git a/drivers/char/ipmi/ipmi_powernv.c > b/drivers/char/ipmi/ipmi_powernv.c >>>>> index 9b409c0..637486d 100644 >>>>> --- a/drivers/char/ipmi/ipmi_powernv.c >>>>> +++ b/drivers/char/ipmi/ipmi_powernv.c >>>>> @@ -143,9 +143,16 @@ static int ipmi_powernv_recv(struct > ipmi_smi_powernv *smi) >>>>> pr_devel("%s: -> %d (size %lld)\n", __func__, >>>>> rc, rc == 0 ? size : 0); >>>>> if (rc) { >>>>> - spin_unlock_irqrestore(&smi->msg_lock, flags); >>>>> - ipmi_free_smi_msg(msg); >>>>> - return 0; >>>>> + /* If came via the poll, and response was not yet ready */ >>>>> + if (rc == OPAL_EMPTY) { >>>>> + spin_unlock_irqrestore(&smi->msg_lock, flags); >>>>> + return 0; >>>>> + } else { >>>>> + smi->cur_msg = NULL; >>>>> + spin_unlock_irqrestore(&smi->msg_lock, flags); >>>>> + send_error_reply(smi, msg, IPMI_ERR_UNSPECIFIED); >>>>> + return 0; >>>>> + } >>>>> } >>>>> >>>>> if (size < sizeof(*opal_msg)) { >>>>> >>> >>> >>> _______________________________________________ >>> Linuxppc-dev mailing list >>> Linuxppc-dev@lists.ozlabs.org >>> https://lists.ozlabs.org/listinfo/linuxppc-dev [-- Attachment #2: Type: text/html, Size: 6133 bytes --] ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2015-07-29 6:09 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2015-07-16 11:16 [PATCH] ipmi/powernv: Fix potential invalid pointer dereference Neelesh Gupta
2015-07-16 15:01 ` Corey Minyard
2015-07-17 8:42 ` Neelesh Gupta
[not found] ` <55B7342F.8080703@linux.vnet.ibm.com>
2015-07-28 17:51 ` Alistair Popple
2015-07-29 6:05 ` Neelesh Gupta
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).