| Message ID | 1509949271-36280-1-git-send-email-longpeng2@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
> -----Original Message----- > From: longpeng > Sent: Monday, November 06, 2017 2:21 PM > To: berrange@redhat.com; pbonzini@redhat.com; Gonglei (Arei) > Cc: longpeng; qemu-devel@nongnu.org > Subject: [PATCH] crypto: afalg: fix a NULL pointer dereference > > Test-crypto-hash calls qcrypto_hash_bytesv/digest/base64 with > errp=NULL, this will cause a NULL poniter deference if afalg_driver > doesn't support requested algos: > ret = qcrypto_hash_afalg_driver.hash_bytesv(alg, iov, niov, > result, resultlen, > errp); > if (ret == 0) { > return ret; > } > > error_free(*errp); // <--- here > > So we must check 'errp & *errp' before dereference. > > Signed-off-by: Longpeng(Mike) <longpeng2@huawei.com> > --- Reported-by: Paolo Bonzini <pbonzini@redhat.com> Reviewed-by: Gonglei <arei.gonglei@huawei.com> Thanks, -Gonglei > crypto/hash.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/crypto/hash.c b/crypto/hash.c > index ac59c63..c464c78 100644 > --- a/crypto/hash.c > +++ b/crypto/hash.c > @@ -60,7 +60,9 @@ int qcrypto_hash_bytesv(QCryptoHashAlgorithm alg, > * TODO: > * Maybe we should treat some afalg errors as fatal > */ > - error_free(*errp); > + if (errp && *errp) { > + error_free(*errp); > + } > #endif > > return qcrypto_hash_lib_driver.hash_bytesv(alg, iov, niov, > -- > 1.8.3.1 >
On 11/06/2017 12:21 AM, Longpeng(Mike) wrote: > Test-crypto-hash calls qcrypto_hash_bytesv/digest/base64 with > errp=NULL, this will cause a NULL poniter deference if afalg_driver s/poniter deference/pointer dereference/ > doesn't support requested algos: > ret = qcrypto_hash_afalg_driver.hash_bytesv(alg, iov, niov, > result, resultlen, > errp); > if (ret == 0) { > return ret; > } > > error_free(*errp); // <--- here > > So we must check 'errp & *errp' before dereference. No, if we are going to blindly ignore the error from the hash_bytesv() call, then we should pass NULL rather than errp. > > Signed-off-by: Longpeng(Mike) <longpeng2@huawei.com> > --- > crypto/hash.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/crypto/hash.c b/crypto/hash.c > index ac59c63..c464c78 100644 > --- a/crypto/hash.c > +++ b/crypto/hash.c > @@ -60,7 +60,9 @@ int qcrypto_hash_bytesv(QCryptoHashAlgorithm alg, > * TODO: > * Maybe we should treat some afalg errors as fatal > */ > - error_free(*errp); > + if (errp && *errp) { > + error_free(*errp); > + } Any code that uses 'if (errp && *errp)' is suspicious; I think you need a v2 that doesn't try to set errp in the first place, rather than cleaning it up to ignore it after the fact.
On Mon, Nov 06, 2017 at 02:21:11PM +0800, Longpeng(Mike) wrote: > Test-crypto-hash calls qcrypto_hash_bytesv/digest/base64 with > errp=NULL, this will cause a NULL poniter deference if afalg_driver > doesn't support requested algos: > ret = qcrypto_hash_afalg_driver.hash_bytesv(alg, iov, niov, > result, resultlen, > errp); > if (ret == 0) { > return ret; > } > > error_free(*errp); // <--- here > > So we must check 'errp & *errp' before dereference. Only errp needs to be checked. It's okay to invoke error_free(NULL): void error_free(Error *err) { if (err) {
On 2017/11/7 1:18, Stefan Hajnoczi wrote: > On Mon, Nov 06, 2017 at 02:21:11PM +0800, Longpeng(Mike) wrote: >> Test-crypto-hash calls qcrypto_hash_bytesv/digest/base64 with >> errp=NULL, this will cause a NULL poniter deference if afalg_driver >> doesn't support requested algos: >> ret = qcrypto_hash_afalg_driver.hash_bytesv(alg, iov, niov, >> result, resultlen, >> errp); >> if (ret == 0) { >> return ret; >> } >> >> error_free(*errp); // <--- here >> >> So we must check 'errp & *errp' before dereference. > > Only errp needs to be checked. It's okay to invoke error_free(NULL): > > void error_free(Error *err) > { > if (err) { Ah yes, thanks for your note :) I'll pick another approach to fix this bug.
On 2017/11/7 1:00, Eric Blake wrote: > On 11/06/2017 12:21 AM, Longpeng(Mike) wrote: >> Test-crypto-hash calls qcrypto_hash_bytesv/digest/base64 with >> errp=NULL, this will cause a NULL poniter deference if afalg_driver > > s/poniter deference/pointer dereference/ > OK. >> doesn't support requested algos: >> ret = qcrypto_hash_afalg_driver.hash_bytesv(alg, iov, niov, >> result, resultlen, >> errp); >> if (ret == 0) { >> return ret; >> } >> >> error_free(*errp); // <--- here >> >> So we must check 'errp & *errp' before dereference. > > No, if we are going to blindly ignore the error from the hash_bytesv() > call, then we should pass NULL rather than errp. > The 'errp' in this palce is convenient for debug, it can tell us the reason for failure without stepping into afalg_driver's hash_bytesv(). >> >> Signed-off-by: Longpeng(Mike) <longpeng2@huawei.com> >> --- >> crypto/hash.c | 4 +++- >> 1 file changed, 3 insertions(+), 1 deletion(-) >> >> diff --git a/crypto/hash.c b/crypto/hash.c >> index ac59c63..c464c78 100644 >> --- a/crypto/hash.c >> +++ b/crypto/hash.c >> @@ -60,7 +60,9 @@ int qcrypto_hash_bytesv(QCryptoHashAlgorithm alg, >> * TODO: >> * Maybe we should treat some afalg errors as fatal >> */ >> - error_free(*errp); >> + if (errp && *errp) { >> + error_free(*errp); >> + } > > Any code that uses 'if (errp && *errp)' is suspicious; I think you need I see, thanks a lot. > a v2 that doesn't try to set errp in the first place, rather than > cleaning it up to ignore it after the fact. > I answered above. How about the following approach? int qcrypto_hash_bytesv(...Error **errp) { #ifdef CONFIG_AF_ALG int ret; Error *err2 = NULL; ret = qcrypto_hash_afalg_driver.hash_bytesv(alg, iov, niov, result, resultlen, &err2); if (ret == 0) { return ret; } error_free(*err2); #endif return qcrypto_hash_lib_driver.hash_bytesv(alg, iov, niov, result, resultlen, errp); } I'll send a V2 if there's no objection, otherwise I'll pick your suggested approach. :)
On Tue, Nov 07, 2017 at 10:27:10AM +0800, Longpeng (Mike) wrote: > > > On 2017/11/7 1:00, Eric Blake wrote: > > > On 11/06/2017 12:21 AM, Longpeng(Mike) wrote: > >> Test-crypto-hash calls qcrypto_hash_bytesv/digest/base64 with > >> errp=NULL, this will cause a NULL poniter deference if afalg_driver > > > > s/poniter deference/pointer dereference/ > > > > OK. > > >> doesn't support requested algos: > >> ret = qcrypto_hash_afalg_driver.hash_bytesv(alg, iov, niov, > >> result, resultlen, > >> errp); > >> if (ret == 0) { > >> return ret; > >> } > >> > >> error_free(*errp); // <--- here > >> > >> So we must check 'errp & *errp' before dereference. > > > > No, if we are going to blindly ignore the error from the hash_bytesv() > > call, then we should pass NULL rather than errp. > > > > The 'errp' in this palce is convenient for debug, it can tell us the reason for > failure without stepping into afalg_driver's hash_bytesv(). It doesn't do anything useful for debug, because we are just immediately throwing away the error without printing it anywhere. Just pass NULL into the hash_bytesv call above. Regards, Daniel
On 2017/11/7 17:16, Daniel P. Berrange wrote: > On Tue, Nov 07, 2017 at 10:27:10AM +0800, Longpeng (Mike) wrote: >> >> >> On 2017/11/7 1:00, Eric Blake wrote: >> >>> On 11/06/2017 12:21 AM, Longpeng(Mike) wrote: >>>> Test-crypto-hash calls qcrypto_hash_bytesv/digest/base64 with >>>> errp=NULL, this will cause a NULL poniter deference if afalg_driver >>> >>> s/poniter deference/pointer dereference/ >>> >> >> OK. >> >>>> doesn't support requested algos: >>>> ret = qcrypto_hash_afalg_driver.hash_bytesv(alg, iov, niov, >>>> result, resultlen, >>>> errp); >>>> if (ret == 0) { >>>> return ret; >>>> } >>>> >>>> error_free(*errp); // <--- here >>>> >>>> So we must check 'errp & *errp' before dereference. >>> >>> No, if we are going to blindly ignore the error from the hash_bytesv() >>> call, then we should pass NULL rather than errp. >>> >> >> The 'errp' in this palce is convenient for debug, it can tell us the reason for >> failure without stepping into afalg_driver's hash_bytesv(). > > It doesn't do anything useful for debug, because we are just immediately > throwing away the error without printing it anywhere. Just pass NULL into > the hash_bytesv call above. > OK.. Afalg-backend cipher/hmac has the same usage, so maybe I need to correct all of them. > Regards, > Daniel
diff --git a/crypto/hash.c b/crypto/hash.c index ac59c63..c464c78 100644 --- a/crypto/hash.c +++ b/crypto/hash.c @@ -60,7 +60,9 @@ int qcrypto_hash_bytesv(QCryptoHashAlgorithm alg, * TODO: * Maybe we should treat some afalg errors as fatal */ - error_free(*errp); + if (errp && *errp) { + error_free(*errp); + } #endif return qcrypto_hash_lib_driver.hash_bytesv(alg, iov, niov,
Test-crypto-hash calls qcrypto_hash_bytesv/digest/base64 with errp=NULL, this will cause a NULL poniter deference if afalg_driver doesn't support requested algos: ret = qcrypto_hash_afalg_driver.hash_bytesv(alg, iov, niov, result, resultlen, errp); if (ret == 0) { return ret; } error_free(*errp); // <--- here So we must check 'errp & *errp' before dereference. Signed-off-by: Longpeng(Mike) <longpeng2@huawei.com> --- crypto/hash.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)