linux-kernel - Re: [PATCH 1/2] crypto: lrw - Fix an error handling path in 'create()'

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 10 Oct 2017 10:32:11 +0200
From:   walter harms <wharms@....de>
To:     Christophe JAILLET <christophe.jaillet@...adoo.fr>
CC:     herbert@...dor.apana.org.au, davem@...emloft.net,
        linux-crypto@...r.kernel.org, linux-kernel@...r.kernel.org,
        kernel-janitors@...r.kernel.org
Subject: Re: [PATCH 1/2] crypto: lrw - Fix an error handling path in 'create()'



Am 10.10.2017 08:05, schrieb Christophe JAILLET:
> Le 09/10/2017 à 23:22, walter harms a écrit :
>> Am 08.10.2017 11:39, schrieb Christophe JAILLET:
>>> All error handling paths 'goto err_drop_spawn' except this one.
>>> In order to avoid some resources leak, we should do it as well here.
>>>
>>> Fixes: 700cb3f5fe75 ("crypto: lrw - Convert to skcipher")
>>> Signed-off-by: Christophe JAILLET <christophe.jaillet@...adoo.fr>
>>> ---
>>>   crypto/lrw.c | 6 ++++--
>>>   1 file changed, 4 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/crypto/lrw.c b/crypto/lrw.c
>>> index a8bfae4451bf..eb681e9fe574 100644
>>> --- a/crypto/lrw.c
>>> +++ b/crypto/lrw.c
>>> @@ -610,8 +610,10 @@ static int create(struct crypto_template *tmpl,
>>> struct rtattr **tb)
>>>           ecb_name[len - 1] = 0;
>>>             if (snprintf(inst->alg.base.cra_name, CRYPTO_MAX_ALG_NAME,
>>> -                 "lrw(%s)", ecb_name) >= CRYPTO_MAX_ALG_NAME)
>>     this check can be done more easy,
>>     the length of ecb_name is len
>>     the length of inst->alg.base.cra_name is CRYPTO_MAX_ALG_NAME
>>     if CRYPTO_MAX_ALG_NAME-len < "lrw()" < 5
>>         no need to involve snprintf()
>>
>>     just my 2 cents
>>     re,
>>           wh
> It does not only check for the length, it also copies some data.
> The test should be read: "If the copy succeeds (i.e if there is enough
> space for the copy to succeed)", and not "if the string is too long".
> IMHO, the snprintf is just fine here.


under "normal" circumstance i would say "does not matter" when
something ends up as crippled string but in case of crypto sameone
(the maintainer) needs to be careful. I have no idea about the
consequences, i can only point to strange looking things and
say "be careful".

re,
 wh

> 
> CJ
>>> -            return -ENAMETOOLONG;
>>> +                 "lrw(%s)", ecb_name) >= CRYPTO_MAX_ALG_NAME) {
>>> +            err = -ENAMETOOLONG;
>>> +            goto err_drop_spawn;
>>> +        }
>>>       }
>>>         inst->alg.base.cra_flags = alg->base.cra_flags &
>>> CRYPTO_ALG_ASYNC;
> 
> 
> -- 
> To unsubscribe from this list: send the line "unsubscribe
> kernel-janitors" in
> the body of a message to majordomo@...r.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>