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

[Stephan Mueller <smueller@chronox.de>]

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