Re: [PATCH v2 20/21] appendedsig: The grub command's trusted and distrusted support

[sudhakar <sudhakar@linux.ibm.com>]

Hi Gary Lin,
Thank you so much for a review!. I wil fix the bug in the code mentioned 
by you.

Thanks,
Sudhakar Kuppusmay

On 2025-04-17 13:13, Gary Lin wrote:
> On Thu, Mar 27, 2025 at 01:02:41AM +0530, Sudhakar Kuppusamy wrote:
>> To support the following trusted and distrusted commands
>> 
>>   1. trusted_list:
>>         It will show the list of trusted certificates and binary 
>> hashes
>>   2. distrusted_list:
>>         It will show the list of distrusted certificates and 
>> binary/certificate hashes
>>   3. trusted_certificate:
>>         It will add the trusted certificate to the trusted list
> There are a few bugs in grub_cmd_distrusted_cert().
> 
>>   4. trusted_signature:
>>         It will add the certificate/binary hash to the trusted list
>>   5. distrusted_certificate:
>>         It will remove the trusted certificate from trsuted list
>>   6. distrusted_signature:
>>         It will add the certificate/binary hash to the distrsuted list
>> 
>> Note:-
>>    The addition/deletion of trusted certificates and binary hashes
>> are not allowed in grub command prompt while secure boot is enabled.
>> 
>> Signed-off-by: Sudhakar Kuppusamy <sudhakar@linux.ibm.com>
>> Reviewed-by: Avnish Chouhan <avnish@linux.ibm.com>
>> ---
>>  grub-core/commands/appendedsig/appendedsig.c | 518 
>> +++++++++++++------
>>  1 file changed, 351 insertions(+), 167 deletions(-)
>> 
>> diff --git a/grub-core/commands/appendedsig/appendedsig.c 
>> b/grub-core/commands/appendedsig/appendedsig.c
>> index 5631f0ab4..6f665aab2 100644
>> --- a/grub-core/commands/appendedsig/appendedsig.c
>> +++ b/grub-core/commands/appendedsig/appendedsig.c
>> @@ -117,6 +117,36 @@ static enum
> 
> ----->8-----
> 
>> +
>> +static grub_err_t
>> +grub_cmd_distrusted_cert (grub_command_t cmd __attribute__((unused)), 
>> int argc, char **args)
>> +{
>> +  grub_size_t cert_num = 0, i = 1;
>> +  struct x509_certificate *current_cert = db.keys;
>> +  struct x509_certificate *previous_cert = db.keys;
>> +
>> +  if (argc != 1)
>> +    {
>> +      grub_printf (N_("trusted certificate number is expected\n"
>> +                      "Example:\n\tdistrusted_certificate 
>> <CERT_NUMER>\n"));
>> +      return GRUB_ERR_BAD_ARGUMENT;
>> +    }
>> +
>> +  if (check_sigs == check_sigs_forced)
>> +    {
>> +      grub_printf ("Warning: since secure boot is enabled, "
>> +                   "removing of trusted certificate is not 
>> permitted!\n");
>> +      return grub_errno;
>> +    }
>> +
>> +  cert_num = grub_strtoul (args[0], NULL, 10);
>> +  if (cert_num < 1)
>> +    return grub_error (GRUB_ERR_BAD_ARGUMENT,
>> +                       N_("trusted certificate number should to begin 
>> with 1"));
>> +
>> +  if (cert_num > db.key_entries)
>> +    return grub_error (GRUB_ERR_BAD_ARGUMENT,
>> +                       N_("trusted certificate number should not 
>> exceed %" PRIuGRUB_SIZE ""),
>> +                       db.key_entries);
>> +  else if (cert_num < db.key_entries)
>> +    return grub_error (GRUB_ERR_BAD_ARGUMENT,
>> +                       N_("there is no certificate on the trusted 
>> list. so, not permitted"));
> The 'else if' looks wrong since it's totally fine to have a cert number
> smaller than db.key_entries. Per the error message, it should be
> 'db.key_entries == 0'.
> 
>> +
>> +  for (i = 1; i < db.key_entries; i++)
> It has to be 'i <= db.key_entries'. Otherwise, the last certificate in
> the list is always ignored.
> 
>> +    {
>> +      if (cert_num == 1)
>>          {
>> -          grub_printf ("%02x:", cert->serial[i]);
>> +          previous_cert = current_cert->next;
> Changing 'previous_cert' here won't affect 'db.keys', so the next user
> of 'db.keys' will get 'current_cert' instead of 'current_cert->next',
> and this leads to crash. I'd suggest to add:
> 
> 	     db.keys = current_cert->next;
> 
>> +          break;
>> +        }
>> +      else if (cert_num == i)
>> +        {
>> +          previous_cert->next = current_cert->next;
>> +          break;
>>          }
>> -      grub_printf ("%02x\n", cert->serial[cert->serial_len - 1]);
>> 
>> -      grub_printf ("\tCN: %s\n\n", cert->subject);
>> -      cert_num++;
>> +      previous_cert = current_cert;
>> +      current_cert = current_cert->next;
>>      }
>> 
>> +  certificate_release (current_cert);
>> +  grub_free (current_cert);
>> +
> 'db.key_entries' wasn't updated in the whole function.
> 
>      db.key_entries--;
> 
>>    return GRUB_ERR_NONE;
>>  }
>> 
> 
> Cheers,
> 
> Gary Lin

_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel