On 3/19/24 2:39 AM, Jarkko Sakkinen wrote:
[...]
>> With the current code, in case all NULLs are passed in id_{0,1,2},
>
> "current code" is not unambigious reference of any part of the kernel
> tree. Please just write down the function name instead.
>
>> the kernel will first print out a WARNING and then have an oops
>> because id_2 gets dereferenced anyway.
>
> Would be more exact":
>
> s/print out a WARNING/emit WARN/
Well, technically calling WARN_ON() it prints out WARNING: ... --
hence the wording... :-)
>> Note that WARN_ON() is also considered harmful by Greg Kroah-
>> Hartman since it causes the Android kernels to panic as they
>> get booted with the panic_on_warn option.
As it turns out, not all Android kernels really do this thging
(at least the Samsung's ones do, according to Greg)...
> Despite full respect to Greg, and agreeing what he had said about
> the topic (which you are lacking lore link meaning that in all
> cases the current description is incomplete), the only thing that
> should be documented should be that since WARN_ON() can emit
> panic when panic_on_warn is set in the *kernel command-line*
> (not "option") this condition should be relaxed.
Linus' opinion seems to be that the people using panic_on_warn
get what they deserve -- see:
https://lore.kernel.org/all/CAHk-=wgF7K2gSSpy=m_=K3Nov4zaceUX9puQf1TjkTJLA2XC_g@xxxxxxxxxxxxxx/
>> Found by Linux Verification Center (linuxtesting.org) with Svace.
>
> I'm not sure if this should be part of the commit message.
>
>>
>> Fixes: 7d30198ee24f ("keys: X.509 public key issuer lookup without AKID")
>> Suggested-by: Sergey Shtylyov <s.shtylyov@xxxxxx>
>
> Should be reported-by.
>
>> Signed-off-by: Roman Smirnov <r.smirnov@xxxxxx>
>> Reviewed-by: Sergey Shtylyov <s.shtylyov@xxxxxx>
>> ---
>> crypto/asymmetric_keys/asymmetric_type.c | 6 +++---
>> 1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/crypto/asymmetric_keys/asymmetric_type.c b/crypto/asymmetric_keys/asymmetric_type.c
>> index a5da8ccd353e..f5cbd6ff14e2 100644
>> --- a/crypto/asymmetric_keys/asymmetric_type.c
>> +++ b/crypto/asymmetric_keys/asymmetric_type.c
>> @@ -60,17 +60,17 @@ struct key *find_asymmetric_key(struct key *keyring,
>> char *req, *p;
>> int len;
>>
>> - WARN_ON(!id_0 && !id_1 && !id_2);
>> -
>
> Weird, I recall discussing about this issue in the past. Unfortunately
> could not find the thread from lore.
There was also that (denied) patch:
https://lore.kernel.org/all/20240414170850.148122-1-elder@xxxxxxxxxx/
> Anyway I agree with the code change.
>
>> if (id_0) {
>> lookup = id_0->data;
>> len = id_0->len;
>> } else if (id_1) {
>> lookup = id_1->data;
>> len = id_1->len;
>> - } else {
>> + } else if (id_2) {
>> lookup = id_2->data;
>> len = id_2->len;
>> + } else {
We can perhaps place the WARN_ON(1) call here instead of where it
is now...
>> + return ERR_PTR(-EINVAL);
>> }
>>
>> /* Construct an identifier "id:<keyid>". */
> BR, Jarkko
MBR, Sergey
