| Message ID | 1517738929-229785-1-git-send-email-cgxu519@icloud.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Sun, Feb 4, 2018 at 11:08 AM, Chengguang Xu <cgxu519@icloud.com> wrote: > There are many reasons of failure from get_secret(), > so it's necessary to add a check before calling crypto_free_skcipher() > in case of NULL pointer dereference. > > Signed-off-by: Chengguang Xu <cgxu519@icloud.com> > --- > net/ceph/crypto.c | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) > > diff --git a/net/ceph/crypto.c b/net/ceph/crypto.c > index bf9d079..bb1dc2d 100644 > --- a/net/ceph/crypto.c > +++ b/net/ceph/crypto.c > @@ -136,8 +136,10 @@ void ceph_crypto_key_destroy(struct ceph_crypto_key *key) > if (key) { > kfree(key->key); > key->key = NULL; > - crypto_free_skcipher(key->tfm); > - key->tfm = NULL; > + if (key->tfm) { > + crypto_free_skcipher(key->tfm); > + key->tfm = NULL; > + } > } > } Hi Chengguang, crypto_free_skcipher() can handle a NULL pointer just fine: void crypto_destroy_tfm(void *mem, struct crypto_tfm *tfm) { struct crypto_alg *alg; if (unlikely(!mem)) return; Thanks, Ilya -- To unsubscribe from this list: send the line "unsubscribe ceph-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Thanks, Chengguang. > 在 2018年2月5日，下午7:51，Ilya Dryomov <idryomov@gmail.com> 写道： > > On Sun, Feb 4, 2018 at 11:08 AM, Chengguang Xu <cgxu519@icloud.com> wrote: >> There are many reasons of failure from get_secret(), >> so it's necessary to add a check before calling crypto_free_skcipher() >> in case of NULL pointer dereference. >> >> Signed-off-by: Chengguang Xu <cgxu519@icloud.com> >> --- >> net/ceph/crypto.c | 6 ++++-- >> 1 file changed, 4 insertions(+), 2 deletions(-) >> >> diff --git a/net/ceph/crypto.c b/net/ceph/crypto.c >> index bf9d079..bb1dc2d 100644 >> --- a/net/ceph/crypto.c >> +++ b/net/ceph/crypto.c >> @@ -136,8 +136,10 @@ void ceph_crypto_key_destroy(struct ceph_crypto_key *key) >> if (key) { >> kfree(key->key); >> key->key = NULL; >> - crypto_free_skcipher(key->tfm); >> - key->tfm = NULL; >> + if (key->tfm) { >> + crypto_free_skcipher(key->tfm); >> + key->tfm = NULL; >> + } >> } >> } > > Hi Chengguang, > > crypto_free_skcipher() can handle a NULL pointer just fine: > > void crypto_destroy_tfm(void *mem, struct crypto_tfm *tfm) > { > struct crypto_alg *alg; > > if (unlikely(!mem)) > return; Hi Ilya, How about crypto_skcipher_tfm() in below call stack? ceph_crypto_key_destroy() -> crypto_free_skcipher() -> crypto_destroy_tfm() -> crypto_skcipher_tfm() static inline struct crypto_tfm *crypto_skcipher_tfm( struct crypto_skcipher *tfm) { return &tfm->base; } -- To unsubscribe from this list: send the line "unsubscribe ceph-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Feb 5, 2018 at 1:06 PM, Chengguang Xu <cgxu519@icloud.com> wrote: > > > Thanks, > Chengguang. > > > >> 在 2018年2月5日，下午7:51，Ilya Dryomov <idryomov@gmail.com> 写道： >> >> On Sun, Feb 4, 2018 at 11:08 AM, Chengguang Xu <cgxu519@icloud.com> wrote: >>> There are many reasons of failure from get_secret(), >>> so it's necessary to add a check before calling crypto_free_skcipher() >>> in case of NULL pointer dereference. >>> >>> Signed-off-by: Chengguang Xu <cgxu519@icloud.com> >>> --- >>> net/ceph/crypto.c | 6 ++++-- >>> 1 file changed, 4 insertions(+), 2 deletions(-) >>> >>> diff --git a/net/ceph/crypto.c b/net/ceph/crypto.c >>> index bf9d079..bb1dc2d 100644 >>> --- a/net/ceph/crypto.c >>> +++ b/net/ceph/crypto.c >>> @@ -136,8 +136,10 @@ void ceph_crypto_key_destroy(struct ceph_crypto_key *key) >>> if (key) { >>> kfree(key->key); >>> key->key = NULL; >>> - crypto_free_skcipher(key->tfm); >>> - key->tfm = NULL; >>> + if (key->tfm) { >>> + crypto_free_skcipher(key->tfm); >>> + key->tfm = NULL; >>> + } >>> } >>> } >> >> Hi Chengguang, >> >> crypto_free_skcipher() can handle a NULL pointer just fine: >> >> void crypto_destroy_tfm(void *mem, struct crypto_tfm *tfm) >> { >> struct crypto_alg *alg; >> >> if (unlikely(!mem)) >> return; > > Hi Ilya, > > How about crypto_skcipher_tfm() in below call stack? > > ceph_crypto_key_destroy() -> crypto_free_skcipher() -> crypto_destroy_tfm() -> crypto_skcipher_tfm() > > static inline struct crypto_tfm *crypto_skcipher_tfm( > struct crypto_skcipher *tfm) > { > return &tfm->base; > } That's an address-of, not a dereference. When tfm is NULL, the result is the offset of base in crypto_skcipher. Thanks, Ilya -- To unsubscribe from this list: send the line "unsubscribe ceph-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Yeah, you are right, I misread that bit. Should I resend second patch? Thanks, Chengguang. > 在 2018年2月5日，下午9:11，Ilya Dryomov <idryomov@gmail.com> 写道： > > On Mon, Feb 5, 2018 at 1:06 PM, Chengguang Xu <cgxu519@icloud.com> wrote: >> >> >> Thanks, >> Chengguang. >> >> >> >>> 在 2018年2月5日，下午7:51，Ilya Dryomov <idryomov@gmail.com> 写道： >>> >>> On Sun, Feb 4, 2018 at 11:08 AM, Chengguang Xu <cgxu519@icloud.com> wrote: >>>> There are many reasons of failure from get_secret(), >>>> so it's necessary to add a check before calling crypto_free_skcipher() >>>> in case of NULL pointer dereference. >>>> >>>> Signed-off-by: Chengguang Xu <cgxu519@icloud.com> >>>> --- >>>> net/ceph/crypto.c | 6 ++++-- >>>> 1 file changed, 4 insertions(+), 2 deletions(-) >>>> >>>> diff --git a/net/ceph/crypto.c b/net/ceph/crypto.c >>>> index bf9d079..bb1dc2d 100644 >>>> --- a/net/ceph/crypto.c >>>> +++ b/net/ceph/crypto.c >>>> @@ -136,8 +136,10 @@ void ceph_crypto_key_destroy(struct ceph_crypto_key *key) >>>> if (key) { >>>> kfree(key->key); >>>> key->key = NULL; >>>> - crypto_free_skcipher(key->tfm); >>>> - key->tfm = NULL; >>>> + if (key->tfm) { >>>> + crypto_free_skcipher(key->tfm); >>>> + key->tfm = NULL; >>>> + } >>>> } >>>> } >>> >>> Hi Chengguang, >>> >>> crypto_free_skcipher() can handle a NULL pointer just fine: >>> >>> void crypto_destroy_tfm(void *mem, struct crypto_tfm *tfm) >>> { >>> struct crypto_alg *alg; >>> >>> if (unlikely(!mem)) >>> return; >> >> Hi Ilya, >> >> How about crypto_skcipher_tfm() in below call stack? >> >> ceph_crypto_key_destroy() -> crypto_free_skcipher() -> crypto_destroy_tfm() -> crypto_skcipher_tfm() >> >> static inline struct crypto_tfm *crypto_skcipher_tfm( >> struct crypto_skcipher *tfm) >> { >> return &tfm->base; >> } > > That's an address-of, not a dereference. When tfm is NULL, the result > is the offset of base in crypto_skcipher. > > Thanks, > > Ilya > -- > To unsubscribe from this list: send the line "unsubscribe ceph-devel" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe ceph-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Feb 5, 2018 at 2:40 PM, Chengguang Xu <cgxu519@icloud.com> wrote:
> Yeah, you are right, I misread that bit. Should I resend second patch?
Yes, it doesn't apply.
Thanks,
Ilya
--
To unsubscribe from this list: send the line "unsubscribe ceph-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/net/ceph/crypto.c b/net/ceph/crypto.c index bf9d079..bb1dc2d 100644 --- a/net/ceph/crypto.c +++ b/net/ceph/crypto.c @@ -136,8 +136,10 @@ void ceph_crypto_key_destroy(struct ceph_crypto_key *key) if (key) { kfree(key->key); key->key = NULL; - crypto_free_skcipher(key->tfm); - key->tfm = NULL; + if (key->tfm) { + crypto_free_skcipher(key->tfm); + key->tfm = NULL; + } } }
There are many reasons of failure from get_secret(), so it's necessary to add a check before calling crypto_free_skcipher() in case of NULL pointer dereference. Signed-off-by: Chengguang Xu <cgxu519@icloud.com> --- net/ceph/crypto.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)