* [Qemu-devel] [PATCH] crypto: afalg: fix a NULL pointer dereference
@ 2017-11-06 6:21 Longpeng(Mike)
2017-11-06 10:21 ` Gonglei (Arei)
` (2 more replies)
0 siblings, 3 replies; 8+ messages in thread
From: Longpeng(Mike) @ 2017-11-06 6:21 UTC (permalink / raw)
To: berrange, pbonzini, arei.gonglei; +Cc: longpeng2, qemu-devel
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(-)
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
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH] crypto: afalg: fix a NULL pointer dereference
2017-11-06 6:21 [Qemu-devel] [PATCH] crypto: afalg: fix a NULL pointer dereference Longpeng(Mike)
@ 2017-11-06 10:21 ` Gonglei (Arei)
2017-11-06 17:00 ` Eric Blake
2017-11-06 17:18 ` Stefan Hajnoczi
2 siblings, 0 replies; 8+ messages in thread
From: Gonglei (Arei) @ 2017-11-06 10:21 UTC (permalink / raw)
To: longpeng, berrange, pbonzini; +Cc: qemu-devel
> -----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
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH] crypto: afalg: fix a NULL pointer dereference
2017-11-06 6:21 [Qemu-devel] [PATCH] crypto: afalg: fix a NULL pointer dereference Longpeng(Mike)
2017-11-06 10:21 ` Gonglei (Arei)
@ 2017-11-06 17:00 ` Eric Blake
2017-11-07 2:27 ` Longpeng (Mike)
2017-11-06 17:18 ` Stefan Hajnoczi
2 siblings, 1 reply; 8+ messages in thread
From: Eric Blake @ 2017-11-06 17:00 UTC (permalink / raw)
To: Longpeng(Mike), berrange, pbonzini, arei.gonglei
Cc: qemu-devel, Markus Armbruster
[-- Attachment #1: Type: text/plain, Size: 1594 bytes --]
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.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH] crypto: afalg: fix a NULL pointer dereference
2017-11-06 6:21 [Qemu-devel] [PATCH] crypto: afalg: fix a NULL pointer dereference Longpeng(Mike)
2017-11-06 10:21 ` Gonglei (Arei)
2017-11-06 17:00 ` Eric Blake
@ 2017-11-06 17:18 ` Stefan Hajnoczi
2017-11-07 1:13 ` Longpeng (Mike)
2 siblings, 1 reply; 8+ messages in thread
From: Stefan Hajnoczi @ 2017-11-06 17:18 UTC (permalink / raw)
To: Longpeng(Mike); +Cc: berrange, pbonzini, arei.gonglei, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 719 bytes --]
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) {
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH] crypto: afalg: fix a NULL pointer dereference
2017-11-06 17:18 ` Stefan Hajnoczi
@ 2017-11-07 1:13 ` Longpeng (Mike)
0 siblings, 0 replies; 8+ messages in thread
From: Longpeng (Mike) @ 2017-11-07 1:13 UTC (permalink / raw)
To: Stefan Hajnoczi; +Cc: berrange, pbonzini, arei.gonglei, qemu-devel
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.
--
Regards,
Longpeng(Mike)
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH] crypto: afalg: fix a NULL pointer dereference
2017-11-06 17:00 ` Eric Blake
@ 2017-11-07 2:27 ` Longpeng (Mike)
2017-11-07 9:16 ` Daniel P. Berrange
0 siblings, 1 reply; 8+ messages in thread
From: Longpeng (Mike) @ 2017-11-07 2:27 UTC (permalink / raw)
To: Eric Blake
Cc: berrange, pbonzini, arei.gonglei, qemu-devel, Markus Armbruster,
Stefan Hajnoczi
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. :)
--
Regards,
Longpeng(Mike)
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH] crypto: afalg: fix a NULL pointer dereference
2017-11-07 2:27 ` Longpeng (Mike)
@ 2017-11-07 9:16 ` Daniel P. Berrange
2017-11-07 9:32 ` Longpeng (Mike)
0 siblings, 1 reply; 8+ messages in thread
From: Daniel P. Berrange @ 2017-11-07 9:16 UTC (permalink / raw)
To: Longpeng (Mike)
Cc: Eric Blake, pbonzini, arei.gonglei, qemu-devel,
Markus Armbruster, Stefan Hajnoczi
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
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Qemu-devel] [PATCH] crypto: afalg: fix a NULL pointer dereference
2017-11-07 9:16 ` Daniel P. Berrange
@ 2017-11-07 9:32 ` Longpeng (Mike)
0 siblings, 0 replies; 8+ messages in thread
From: Longpeng (Mike) @ 2017-11-07 9:32 UTC (permalink / raw)
To: Daniel P. Berrange
Cc: Eric Blake, pbonzini, arei.gonglei, qemu-devel,
Markus Armbruster, Stefan Hajnoczi
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
--
Regards,
Longpeng(Mike)
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2017-11-07 9:33 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-11-06 6:21 [Qemu-devel] [PATCH] crypto: afalg: fix a NULL pointer dereference Longpeng(Mike)
2017-11-06 10:21 ` Gonglei (Arei)
2017-11-06 17:00 ` Eric Blake
2017-11-07 2:27 ` Longpeng (Mike)
2017-11-07 9:16 ` Daniel P. Berrange
2017-11-07 9:32 ` Longpeng (Mike)
2017-11-06 17:18 ` Stefan Hajnoczi
2017-11-07 1:13 ` Longpeng (Mike)
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.