Date: Wed, 15 Jan 2014 08:25:28 +1100 From: Benno Rice <benno@FreeBSD.org> To: freebsd-security@freebsd.org Subject: Review of an OpenCrypto patch Message-ID: <74B9B73E-1F47-40F6-9506-E042608B8931@FreeBSD.org>
next in thread | raw e-mail | index | archive | help
--Apple-Mail=_E11561FB-F251-497A-B92D-42B5EAAD2AF4
Content-Type: multipart/mixed;
boundary="Apple-Mail=_13016BD9-3918-4286-A160-1650966F8977"
--Apple-Mail=_13016BD9-3918-4286-A160-1650966F8977
Content-Transfer-Encoding: quoted-printable
Content-Type: text/plain;
charset=windows-1252
Hi -security,
I work at EMC Isilon and one of our developers has found a race in =
opencyrpto and provided the attached patch to address it.
The situation as explained to me is that the crypto request queue and =
dequeue operate under CRYPTO_Q_LOCK, along with crypto_invoke and thus =
crypto processing. Meanwhile crypto_newsession (and thus all driver new =
session calls) operate under CRYPTO_DRIVER_LOCK.
This leads to a situation where resizing of the swcr_sessions array in =
swcr_newsession can interfere with the use of that array in =
swcr_process.
The attached patch protects the swcr_sessions array with a new rwlock.
Could somebody give this a look over and let me know if it=92s =
commitable roughly as is or needs some work?
Cheers,
Benno.
--Apple-Mail=_13016BD9-3918-4286-A160-1650966F8977
Content-Disposition: attachment;
filename=patch-117508.3
Content-Type: application/octet-stream;
name="patch-117508.3"
Content-Transfer-Encoding: quoted-printable
Index:=20src/sys/opencrypto/cryptosoft.c=0D=0A=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=0D=0A---=20=
src/sys/opencrypto/cryptosoft.c=09(revision=20377628)=0D=0A+++=20=
src/sys/opencrypto/cryptosoft.c=09(working=20copy)=0D=0A@@=20-35,6=20=
+35,8=20@@=0D=0A=20#include=20<sys/random.h>=0D=0A=20#include=20=
<sys/kernel.h>=0D=0A=20#include=20<sys/uio.h>=0D=0A+#include=20=
<sys/lock.h>=20/*=20Isilon=20bug=20119136=20fix=20*/=0D=0A+#include=20=
<sys/rwlock.h>=20/*=20Isilon=20bug=20119136=20fix=20*/=0D=0A=20=0D=0A=20=
#include=20<crypto/blowfish/blowfish.h>=0D=0A=20#include=20=
<crypto/sha1.h>=0D=0A@@=20-54,6=20+56,7=20@@=0D=0A=20static=09int32_t=20=
swcr_id;=0D=0A=20static=09struct=20swcr_data=20**swcr_sessions=20=3D=20=
NULL;=0D=0A=20static=09u_int32_t=20swcr_sesnum;=0D=0A+static=20=20struct=20=
rwlock=20swcr_sessions_lock;=20/*=20Isilon=20bug=20119136=20fix=20*/=0D=0A=
=20=0D=0A=20u_int8_t=20hmac_ipad_buffer[HMAC_MAX_BLOCK_LEN];=0D=0A=20=
u_int8_t=20hmac_opad_buffer[HMAC_MAX_BLOCK_LEN];=0D=0A@@=20-618,6=20=
+621,7=20@@=0D=0A=20=09if=20(sid=20=3D=3D=20NULL=20||=20cri=20=3D=3D=20=
NULL)=0D=0A=20=09=09return=20EINVAL;=0D=0A=20=0D=0A+=09=
rw_wlock(&swcr_sessions_lock);=20/*=20Isilon=20bug=20119136=20fix=20*/=0D=
=0A=20=09if=20(swcr_sessions)=20{=0D=0A=20=09=09for=20(i=20=3D=201;=20i=20=
<=20swcr_sesnum;=20i++)=0D=0A=20=09=09=09if=20(swcr_sessions[i]=20=3D=3D=20=
NULL)=0D=0A@@=20-640,6=20+644,7=20@@=0D=0A=20=09=09=09=09swcr_sesnum=20=3D=
=200;=0D=0A=20=09=09=09else=0D=0A=20=09=09=09=09swcr_sesnum=20/=3D=202;=0D=
=0A+=09=09=09rw_wunlock(&swcr_sessions_lock);=20/*=20Isilon=20bug=20=
119136=20fix=20*/=0D=0A=20=09=09=09return=20ENOBUFS;=0D=0A=20=09=09}=0D=0A=
=20=0D=0A@@=20-652,8=20+657,8=20@@=0D=0A=20=0D=0A=20=09=09swcr_sessions=20=
=3D=20swd;=0D=0A=20=09}=0D=0A-=0D=0A=20=09swd=20=3D=20&swcr_sessions[i];=0D=
=0A+=09rw_wunlock(&swcr_sessions_lock);=20/*=20Isilon=20bug=20119136=20=
fix=20*/=0D=0A=20=09*sid=20=3D=20i;=0D=0A=20=0D=0A=20=09while=20(cri)=20=
{=0D=0A@@=20-700,7=20+705,7=20@@=0D=0A=20=09=09=09}=0D=0A=20=09=09=09=
(*swd)->sw_exf=20=3D=20txf;=0D=0A=20=09=09=09break;=0D=0A-=09=0D=0A+=0D=0A=
=20=09=09case=20CRYPTO_MD5_HMAC:=0D=0A=20=09=09=09axf=20=3D=20=
&auth_hash_hmac_md5;=0D=0A=20=09=09=09goto=20authcommon;=0D=0A@@=20=
-823,13=20+828,18=20@@=0D=0A=20=09struct=20comp_algo=20*cxf;=0D=0A=20=09=
u_int32_t=20sid=20=3D=20CRYPTO_SESID2LID(tid);=0D=0A=20=0D=0A+=09=
rw_rlock(&swcr_sessions_lock);=20/*=20Isilon=20bug=20119136=20fix=20*/=0D=
=0A=20=09if=20(sid=20>=20swcr_sesnum=20||=20swcr_sessions=20=3D=3D=20=
NULL=20||=0D=0A-=09=20=20=20=20swcr_sessions[sid]=20=3D=3D=20NULL)=0D=0A=
+=09=20=20=20=20swcr_sessions[sid]=20=3D=3D=20NULL)=20{=0D=0A+=09=09=
rw_runlock(&swcr_sessions_lock);=20/*=20Isilon=20bug=20119136=20fix=20*/=0D=
=0A=20=09=09return=20EINVAL;=0D=0A+=09}=0D=0A=20=0D=0A=20=09/*=20=
Silently=20accept=20and=20return=20*/=0D=0A-=09if=20(sid=20=3D=3D=200)=0D=
=0A+=09if=20(sid=20=3D=3D=200)=20{=0D=0A+=09=09=
rw_runlock(&swcr_sessions_lock);=20/*=20Isilon=20bug=20119136=20fix=20*/=0D=
=0A=20=09=09return=200;=0D=0A+=20=20=20=20=20=20=20=20}=0D=0A=20=0D=0A=20=
=09while=20((swd=20=3D=20swcr_sessions[sid])=20!=3D=20NULL)=20{=0D=0A=20=09=
=09swcr_sessions[sid]=20=3D=20swd->sw_next;=0D=0A@@=20-897,6=20+907,7=20=
@@=0D=0A=20=0D=0A=20=09=09FREE(swd,=20M_CRYPTO_DATA);=0D=0A=20=09}=0D=0A=
+=09rw_runlock(&swcr_sessions_lock);=0D=0A=20=09return=200;=0D=0A=20}=0D=0A=
=20=0D=0A@@=20-920,10=20+931,13=20@@=0D=0A=20=09}=0D=0A=20=0D=0A=20=09=
lid=20=3D=20crp->crp_sid=20&=200xffffffff;=0D=0A+=09=
rw_rlock(&swcr_sessions_lock);=20/*=20Isilon=20bug=20119136=20fix=20*/=0D=
=0A=20=09if=20(lid=20>=3D=20swcr_sesnum=20||=20lid=20=3D=3D=200=20||=20=
swcr_sessions[lid]=20=3D=3D=20NULL)=20{=0D=0A+=09=09=
rw_runlock(&swcr_sessions_lock);=20/*=20Isilon=20bug=20119136=20fix=20*/=0D=
=0A=20=09=09crp->crp_etype=20=3D=20ENOENT;=0D=0A=20=09=09goto=20done;=0D=0A=
=20=09}=0D=0A+=09rw_runlock(&swcr_sessions_lock);=20/*=20Isilon=20bug=20=
119136=20fix=20*/=0D=0A=20=0D=0A=20=09/*=20Go=20through=20crypto=20=
descriptors,=20processing=20as=20we=20go=20*/=0D=0A=20=09for=20(crd=20=3D=20=
crp->crp_desc;=20crd;=20crd=20=3D=20crd->crd_next)=20{=0D=0A@@=20-937,10=20=
+951,12=20@@=0D=0A=20=09=09=20*=20XXX=20between=20the=20various=20=
instances=20of=20an=20algorithm=20(so=20we=20can=0D=0A=20=09=09=20*=20=
XXX=20locate=20the=20correct=20crypto=20context).=0D=0A=20=09=09=20*/=0D=0A=
+=09=09rw_rlock(&swcr_sessions_lock);=20/*=20Isilon=20bug=20119136=20fix=20=
*/=0D=0A=20=09=09for=20(sw=20=3D=20swcr_sessions[lid];=0D=0A=20=09=09=20=20=
=20=20sw=20&&=20sw->sw_alg=20!=3D=20crd->crd_alg;=0D=0A=20=09=09=20=20=20=
=20sw=20=3D=20sw->sw_next)=0D=0A=20=09=09=09;=0D=0A+=09=09=
rw_runlock(&swcr_sessions_lock);=20/*=20Isilon=20bug=20119136=20fix=20*/=0D=
=0A=20=0D=0A=20=09=09/*=20No=20such=20context=20?=20*/=0D=0A=20=09=09if=20=
(sw=20=3D=3D=20NULL)=20{=0D=0A@@=20-1017,6=20+1033,7=20@@=0D=0A=20static=20=
int=0D=0A=20swcr_attach(device_t=20dev)=0D=0A=20{=0D=0A+=09=
rw_init(&swcr_sessions_lock,=20"swcr_sessions_lock");=20/*=20Isilon=20=
bug=20119136=20fix=20*/=0D=0A=20=09memset(hmac_ipad_buffer,=20=
HMAC_IPAD_VAL,=20HMAC_MAX_BLOCK_LEN);=0D=0A=20=09=
memset(hmac_opad_buffer,=20HMAC_OPAD_VAL,=20HMAC_MAX_BLOCK_LEN);=0D=0A=20=
=0D=0A@@=20-1057,8=20+1074,11=20@@=0D=0A=20swcr_detach(device_t=20dev)=0D=
=0A=20{=0D=0A=20=09crypto_unregister_all(swcr_id);=0D=0A+=09=
rw_wlock(&swcr_sessions_lock);=0D=0A=20=09if=20(swcr_sessions=20!=3D=20=
NULL)=0D=0A=20=09=09FREE(swcr_sessions,=20M_CRYPTO_DATA);=0D=0A+=09=
rw_wunlock(&swcr_sessions_lock);=0D=0A+=09=
rw_destroy(&swcr_sessions_lock);=0D=0A=20}=0D=0A=20=0D=0A=20static=20=
device_method_t=20swcr_methods[]=20=3D=20{=0D=0A=
--Apple-Mail=_13016BD9-3918-4286-A160-1650966F8977
Content-Transfer-Encoding: 7bit
Content-Type: text/plain;
charset=us-ascii
--Apple-Mail=_13016BD9-3918-4286-A160-1650966F8977--
--Apple-Mail=_E11561FB-F251-497A-B92D-42B5EAAD2AF4
Content-Transfer-Encoding: 7bit
Content-Disposition: attachment;
filename=signature.asc
Content-Type: application/pgp-signature;
name=signature.asc
Content-Description: Message signed with OpenPGP using GPGMail
-----BEGIN PGP SIGNATURE-----
Comment: GPGTools - http://gpgtools.org
iQIcBAEBCgAGBQJS1atIAAoJELja0BJxpLNeHx0P/ies+Dq8AxenoS7nS47pOLPX
i1C4AgNMu8lzBLLfDrhdEWSUp5AEmXte2KLIPr46AnMnnyix1UkJ7LYE26CdjEL8
6fsinOHze8XV8woRM/LPPG2piPJrAlxs+4uGJ+t6/M//pfDaRmkT//vdxTdKu3G3
uSWhOGkQ9D8smCyQfPsll1m+EuuU9IY9CfEKV5iCIHEwsJlUlOWHGhDesK1KUe7R
XvJesb0pHVmQDnMnDoF0euFJcS2/SJGrzJzk2XLrOLq1XXzhJNooFulkMgQwdkRT
uIP0qGxzyfBJ/h/3xxCDI/P72gnceqtaLJHiNDu3Ol3P/mAOfp9mm4V7lcEvfM5P
7SNiq3tlqLdjkUT9v3+qDM5ziPnpzHuOAbIUMFq66P+veicgIHSWT02+bD0POa1K
xDZrKEe2HMCS7QJe0OcTusLvHmNdYyKkG9j+m/Q8j7O59PrOHwJiVSOhcHYvsqmT
HHRByYTRoH/rAAdhyKoGzO6SfvbnTOgjHuTmJ0KKtoKPETB2mak4HNlbW175WODu
8lZKTuuVbMqT+ut2gNPYjAyUnwFCygSArJBgQRRLIuPoHlTskC6wPDyEeznpaXDU
PccNjddPqdX05UDuZSnetVE577lMUxfk0Xassr4Vke/OcGRnp94+2fvAcpHu0RaU
bnf2GGo+zM6Qn2tk4jjp
=t28X
-----END PGP SIGNATURE-----
--Apple-Mail=_E11561FB-F251-497A-B92D-42B5EAAD2AF4--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?74B9B73E-1F47-40F6-9506-E042608B8931>