netdev - Re: Crypto oops in async_chainiv_do

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]

Date:	Wed, 2 Sep 2009 18:47:49 -0500
From:	"Brad Bosch" <bradbosch@...cast.net>
To:	Herbert Xu <herbert@...dor.apana.org.au>
Cc:	Brad Bosch <bradbosch@...cast.net>, linux-crypto@...r.kernel.org,
	netdev@...r.kernel.org, offbase0@...il.com
Subject: Re: Crypto oops in async_chainiv_do_postponed

Herbert Xu writes:
 > On Wed, Sep 02, 2009 at 09:08:38AM -0500, Brad Bosch wrote:
 > > 
 > > Assume the worker thread is executing between the dequeue in
 > > async_chainiv_do_postponed and the clear_bit call in
 > > async_chainiv_schedule_work.  Further assume that we are processing
 > 
 > It cannot.  The worker thread can only execute when it owns
 > the INUSE bit.  In that case do_postponed will never call the
 > schedule_work function.

In the example I cited (one entry in the queue when the worker
function starts), async_chainiv_schedule_work is indeed executed.
(indirectly) by async_chainiv_givencrypt_tail from the worker thread.
I'm sorry I didn't make it more clear that it is that code path I was
talking about.

 > 
 > Perhaps you were misled by the clear_bit call in schedule_work.
 > That is only used if we end up not scheduling the work.

No, I was not misled.  But apparently, I was not clear.  I do
understand how you use the INUSE bit.  I did not say above that
INUSE is not set when the worker thread is running (at least not for
the first part of my example).  If you had read further, you might
have noticed that the following paragraphs showed that indeed I do
understand that INUSE is set in the worker thread as evidenced by
"thread one calls test_and_set_bit which returns 1" I have added one
sentence (marked by **) to my event description below to make my
understanding more clear.  Please read on.

Assume the worker thread is executing between the dequeue in
async_chainiv_do_postponed and the clear_bit call in
async_chainiv_schedule_work.  Further assume that we are processing
the last item on the queue so durring this time, ctx->queue.qlen =
0.  **INUSE is still set at this point.

Meanwhile, three threads enter async_chainiv_givencrypt for the same
ctx at about the same time.

Thread one calls test_and_set_bit which returns 1 and calls
async_cahiniv_postpone_request but suppose it has not yet enqueued.
Now INUSE is set and qlen=0.

Next, the worker thread calls clear_bit in async_chainiv_schedule_work
but it is interrupted before it can call test_and_set_bit.  Now INUSE
is clear and qlen=0

The test_and_set_bit in thread two is called at this moment and
returns 0 and then calls async_chainiv_givencrypt_tail.  Now INUSE is
set and qlen=0.

Thread one now locks the ctx and calls skcipher_enqueue_givcrypt and
unlocks.  Now INUSE is set and qlen=1.

Thread three calls test_and_set_bit which returns 1 and then it clears
INUSE since qlen=1 and it calls postpone with INUSE clear and qlen=1

Now thread three will use ctx->err to hold the return value of
skcipher_enqueue_givcrypt at the same time as thread two uses ctx->err
to hold the return value of crypto_ablkcipher_encrypt!

Did I make a mistake above?  I suspect more bad things can happen as
well in this scenario, but I'm just focusing on the use of ctx->err here.

Thanks

--Brad
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html