linux-kernel - Re: Potential data race in psmouse

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAKdAkRTtxBNstw+XkOVeY5RPqT8hOOOaA0==-nDiSjYYrV+xWQ@mail.gmail.com>
Date:	Fri, 28 Aug 2015 10:51:47 -0700
From:	Dmitry Torokhov <dmitry.torokhov@...il.com>
To:	Dmitry Vyukov <dvyukov@...gle.com>
Cc:	Pali Rohár <pali.rohar@...il.com>,
	Andrey Konovalov <andreyknvl@...gle.com>,
	Hans de Goede <hdegoede@...hat.com>,
	Mathias Gottschlag <mgottschlag@...il.com>,
	Shailendra Verma <shailendra.capricorn@...il.com>,
	Rusty Russell <rusty@...tcorp.com.au>,
	"Luis R. Rodriguez" <mcgrof@...e.com>,
	Thomas Hellstrom <thellstrom@...are.com>,
	"linux-input@...r.kernel.org" <linux-input@...r.kernel.org>,
	LKML <linux-kernel@...r.kernel.org>,
	Kostya Serebryany <kcc@...gle.com>,
	Alexander Potapenko <glider@...gle.com>, ktsan@...glegroups.com
Subject: Re: Potential data race in psmouse_interrupt

On Fri, Aug 28, 2015 at 10:34 AM, Dmitry Vyukov <dvyukov@...gle.com> wrote:
> Hello,
>
> I am looking at this code in __ps2_command again:
>
> /*
> * The reset command takes a long time to execute.
> */
> timeout = msecs_to_jiffies(command == PS2_CMD_RESET_BAT ? 4000 : 500);
>
> timeout = wait_event_timeout(ps2dev->wait,
>     !(READ_ONCE(ps2dev->flags) & PS2_FLAG_CMD1), timeout);
>
> if (smp_load_acquire(&ps2dev->cmdcnt) &&
>     !(smp_load_acquire(&ps2dev->flags) & PS2_FLAG_CMD1)) {
>               timeout = ps2_adjust_timeout(ps2dev, command, timeout);
>               wait_event_timeout(ps2dev->wait,
>                         !(smp_load_acquire(&ps2dev->flags) &
> PS2_FLAG_CMD), timeout);
> }
>
> if (param)
>     for (i = 0; i < receive; i++)
>               param[i] = ps2dev->cmdbuf[(receive - 1) - i];
>
>
> Here are two moments I don't understand:
> 1. The last parameter of ps2_adjust_timeout is timeout in jiffies (it
> is compared against 100ms). However, timeout is assigned to result of
> wait_event_timeout, which returns 0 or 1. This does not make sense to
> me. What am I missing?

The fact that wait_event_timeout can return value greater than one:

 * Returns:
 * 0 if the @condition evaluated to %false after the @timeout elapsed,
 * 1 if the @condition evaluated to %true after the @timeout elapsed,
 * or the remaining jiffies (at least 1) if the @condition evaluated
              ^^^^^^^^^^^^^^^^^^^^^^^^^

> 2. This code pays great attention to timeouts, but in the end I don't
> see how it handles timeouts. That is, if a timeout is happened, we
> still copyout (garbage) from cmdbuf. What am I missing here?

Once upon a time wait_event() did not return positive value when
timeout expired and then condition satisfied. So we just examine the
final state (psmpouse->cmdcnt should be 0 if command actually
succeeded) and even if we copy in garbage nobody should care since
we'll return error in this case.

Thanks.

-- 
Dmitry
--
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/