linux-kernel - Re: [PATCH v2] crypto: AF_ALG - race-free access of encryption flag

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, 29 Nov 2017 07:48:53 +0100
From:   Stephan Mueller <smueller@...onox.de>
To:     Herbert Xu <herbert@...dor.apana.org.au>
Cc:     Eric Biggers <ebiggers3@...il.com>,
        syzbot 
        <bot+3401d9494b9380f7244bcc7fec49680878fccba6@...kaller.appspotmail.com>,
        davem@...emloft.net, linux-crypto@...r.kernel.org,
        linux-kernel@...r.kernel.org, syzkaller-bugs@...glegroups.com
Subject: Re: [PATCH v2] crypto: AF_ALG - race-free access of encryption flag

Am Mittwoch, 29. November 2017, 00:02:40 CET schrieb Herbert Xu:

Hi Herbert,

> > --- a/crypto/algif_aead.c
> > +++ b/crypto/algif_aead.c
> > @@ -110,6 +110,7 @@ static int _aead_recvmsg(struct socket *sock, struct
> > msghdr *msg,> 
> >  	size_t outlen = 0;		/* [out] RX bufs produced by kernel */
> >  	size_t usedpages = 0;		/* [in]  RX bufs to be used from user */
> >  	size_t processed = 0;		/* [in]  TX bufs to be consumed */
> > 
> > +	bool enc = ctx->enc;		/* prevent race if sock lock dropped */
> 
> This is wrong.  You can't fetch ctx->enc before you wait.  It has
> to be done after the wait as otherwise ctx->enc may not even have
> been initialised.

All ctx variables are initialized in aead_accept_parent_nokey. Thus, if no 
sendmsg call is invoked by user space, at least a valid operation is 
performed.
> 
> In fact the position of the wait call is just wrong.  You should only
> ever do at most one wait at the start of the recvmsg call.  Once you
> have enough data from that wait no further waits should be done for
> that recvmsg call.  The original commit had it right with just one
> wait at the start of recvmsg (400c40cf78da).

I am wondering about the wait call inside the while () in af_alg_get_rsgl: 
this has been taken from algif_skcipher. It would seem that moving the wait 
call to the beginning of the recvmsg call would be applicable to skcipher as 
well. I.e. the wait call should be removed from af_alg_get_rsgl entirely and 
placed at the beginning the recvmsg function in both, the aead and skcipher 
code paths.
> 
> Fetching of ctx state should be performed after this wait.
> 
> Cheers,



Ciao
Stephan