| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Thu, 25 Jul 2013 21:32:34 +0200
From: "Rafael J. Wysocki" <rjw@...k.pl>
To: minyard@....org, "Zheng, Lv" <lv.zheng@...el.com>
Cc: "Wysocki, Rafael J" <rafael.j.wysocki@...el.com>,
"Brown, Len" <len.brown@...el.com>,
"Zhao, Yakui" <yakui.zhao@...el.com>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"linux-acpi@...r.kernel.org" <linux-acpi@...r.kernel.org>,
"openipmi-developer@...ts.sourceforge.net"
<openipmi-developer@...ts.sourceforge.net>
Subject: Re: [PATCH 03/13] ACPI/IPMI: Fix race caused by the unprotected ACPI IPMI transfers
On Thursday, July 25, 2013 01:12:38 PM Corey Minyard wrote:
> On 07/25/2013 07:06 AM, Rafael J. Wysocki wrote:
> > On Thursday, July 25, 2013 03:09:35 AM Zheng, Lv wrote:
> >> -stable according to the previous conversation.
> >>
> >>> From: Rafael J. Wysocki [mailto:rjw@...k.pl]
> >>> Sent: Thursday, July 25, 2013 7:38 AM
> >>>
> >>> On Tuesday, July 23, 2013 04:09:15 PM Lv Zheng wrote:
> >>>> This patch fixes races caused by unprotected ACPI IPMI transfers.
> >>>>
> >>>> We can see the following crashes may occur:
> >>>> 1. There is no tx_msg_lock held for iterating tx_msg_list in
> >>>> ipmi_flush_tx_msg() while it is parellel unlinked on failure in
> >>>> acpi_ipmi_space_handler() under protection of tx_msg_lock.
> >>>> 2. There is no lock held for freeing tx_msg in acpi_ipmi_space_handler()
> >>>> while it is parellel accessed in ipmi_flush_tx_msg() and
> >>>> ipmi_msg_handler().
> >>>>
> >>>> This patch enhances tx_msg_lock to protect all tx_msg accesses to
> >>>> solve this issue. Then tx_msg_lock is always held around complete()
> >>>> and tx_msg accesses.
> >>>> Calling smp_wmb() before setting msg_done flag so that messages
> >>>> completed due to flushing will not be handled as 'done' messages while
> >>>> their contents are not vaild.
> >>>>
> >>>> Signed-off-by: Lv Zheng <lv.zheng@...el.com>
> >>>> Cc: Zhao Yakui <yakui.zhao@...el.com>
> >>>> Reviewed-by: Huang Ying <ying.huang@...el.com>
> >>>> ---
> >>>> drivers/acpi/acpi_ipmi.c | 10 ++++++++--
> >>>> 1 file changed, 8 insertions(+), 2 deletions(-)
> >>>>
> >>>> diff --git a/drivers/acpi/acpi_ipmi.c b/drivers/acpi/acpi_ipmi.c index
> >>>> b37c189..527ee43 100644
> >>>> --- a/drivers/acpi/acpi_ipmi.c
> >>>> +++ b/drivers/acpi/acpi_ipmi.c
> >>>> @@ -230,11 +230,14 @@ static void ipmi_flush_tx_msg(struct
> >>> acpi_ipmi_device *ipmi)
> >>>> struct acpi_ipmi_msg *tx_msg, *temp;
> >>>> int count = HZ / 10;
> >>>> struct pnp_dev *pnp_dev = ipmi->pnp_dev;
> >>>> + unsigned long flags;
> >>>>
> >>>> + spin_lock_irqsave(&ipmi->tx_msg_lock, flags);
> >>>> list_for_each_entry_safe(tx_msg, temp, &ipmi->tx_msg_list, head) {
> >>>> /* wake up the sleep thread on the Tx msg */
> >>>> complete(&tx_msg->tx_complete);
> >>>> }
> >>>> + spin_unlock_irqrestore(&ipmi->tx_msg_lock, flags);
> >>>>
> >>>> /* wait for about 100ms to flush the tx message list */
> >>>> while (count--) {
> >>>> @@ -268,13 +271,12 @@ static void ipmi_msg_handler(struct
> >>> ipmi_recv_msg *msg, void *user_msg_data)
> >>>> break;
> >>>> }
> >>>> }
> >>>> - spin_unlock_irqrestore(&ipmi_device->tx_msg_lock, flags);
> >>>>
> >>>> if (!msg_found) {
> >>>> dev_warn(&pnp_dev->dev,
> >>>> "Unexpected response (msg id %ld) is returned.\n",
> >>>> msg->msgid);
> >>>> - goto out_msg;
> >>>> + goto out_lock;
> >>>> }
> >>>>
> >>>> /* copy the response data to Rx_data buffer */ @@ -286,10 +288,14 @@
> >>>> static void ipmi_msg_handler(struct ipmi_recv_msg *msg, void
> >>> *user_msg_data)
> >>>> }
> >>>> tx_msg->rx_len = msg->msg.data_len;
> >>>> memcpy(tx_msg->data, msg->msg.data, tx_msg->rx_len);
> >>>> + /* tx_msg content must be valid before setting msg_done flag */
> >>>> + smp_wmb();
> >>> That's suspicious.
> >>>
> >>> If you need the write barrier here, you'll most likely need a read barrier
> >>> somewhere else. Where's that?
> >> It might depend on whether the content written before the smp_wmb() is used or not by the other side codes under the condition set after the smp_wmb().
> >>
> >> So comment could be treated as 2 parts:
> >> 1. do we need a paired smp_rmb().
> >> 2. do we need a smp_wmb().
> >>
> >> For 1.
> >> If we want a paired smp_rmb(), then it will appear in this function:
> >>
> >> 186 static void acpi_format_ipmi_response(struct acpi_ipmi_msg *msg,
> >> 187 acpi_integer *value, int rem_time)
> >> 188 {
> >> 189 struct acpi_ipmi_buffer *buffer;
> >> 190
> >> 191 /*
> >> 192 * value is also used as output parameter. It represents the response
> >> 193 * IPMI message returned by IPMI command.
> >> 194 */
> >> 195 buffer = (struct acpi_ipmi_buffer *)value;
> >> 196 if (!rem_time && !msg->msg_done) {
> >> 197 buffer->status = ACPI_IPMI_TIMEOUT;
> >> 198 return;
> >> 199 }
> >> 200 /*
> >> 201 * If the flag of msg_done is not set or the recv length is zero, it
> >> 202 * means that the IPMI command is not executed correctly.
> >> 203 * The status code will be ACPI_IPMI_UNKNOWN.
> >> 204 */
> >> 205 if (!msg->msg_done || !msg->rx_len) {
> >> 206 buffer->status = ACPI_IPMI_UNKNOWN;
> >> 207 return;
> >> 208 }
> >> + smp_rmb();
> >> 209 /*
> >> 210 * If the IPMI response message is obtained correctly, the status code
> >> 211 * will be ACPI_IPMI_OK
> >> 212 */
> >> 213 buffer->status = ACPI_IPMI_OK;
> >> 214 buffer->length = msg->rx_len;
> >> 215 memcpy(buffer->data, msg->rx_data, msg->rx_len);
> >> 216 }
> >>
> >> If we don't then there will only be msg content not correctly read from msg->rx_data.
> >> Note that the rx_len is 0 during initialization and will never exceed the sizeof(buffer->data), so the read is safe.
> >>
> >> Being without smp_rmb() is also OK in this case, since:
> >> 1. buffer->data will never be used when buffer->status is not ACPI_IPMI_OK and
> >> 2. the smp_rmb()/smp_wmb() added in this patch will be deleted in [PATCH 07].
> >>
> >> So IMO, we needn't add the smp_rmb(), what do you think of this?
> >>
> >> For 2.
> >> If we don't add smp_wmb() in the ipmi_msg_handler(), then the codes running on other thread in the acpi_format_ipmi_response() may read wrong msg->rx_data (a timeout triggers this function, but when acpi_format_ipmi_response() is entered, the msg->msg_done flag could be seen as 1 but the msg->rx_data is not ready), this is what we want to avoid in this quick fix.
> > Using smp_wmb() without the complementary smp_rmb() doesn't makes sense,
> > because each of them prevents only one flow of control from being
> > speculatively reordered, either by the CPU or by the compiler. If only one
> > of them is used without the other, then the flow of control without the
> > barrier may be reordered in a way that will effectively cancel the effect of
> > the barrier in the second flow of control.
> >
> > So, either we need *both* smp_wmb() and smp_rmb(), or we don't need them at all.
>
> If I understand this correctly, the problem would be if:
>
> rem_time = wait_for_completion_timeout(&tx_msg->tx_complete,
> IPMI_TIMEOUT);
>
> returns on a timeout, then checks msg_done and races with something
> setting msg_done. If that is the case, you would need the smp_rmb()
> before checking msg_done.
I believe so.
> However, the timeout above is unnecessary. You are using
> ipmi_request_settime(), so you can set the timeout when the IPMI command
> fails and returns a failure message. The driver guarantees a return
> message for each request. Just remove the timeout from the completion,
> set the timeout and retries in the ipmi request, and the completion
> should handle the barrier issues.
Good point.
> Plus, from a quick glance at the code, it doesn't look like it will
> properly handle a situation where the timeout occurs and is handled then
> the response comes in later.
Lv, what about this?
Rafael
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists