Re: [PATCH v4 2/3] xsm: consolidate loading the policy buffer

["Daniel P. Smith" <dpsmith@xxxxxxxxxxxxxxxxxxxx>]

On 5/31/22 15:07, Andrew Cooper wrote:
> On 31/05/2022 19:20, Daniel P. Smith wrote:
>> diff --git a/xen/xsm/xsm_policy.c b/xen/xsm/xsm_policy.c
>> index 8dafbc9381..690fd23e9f 100644
>> --- a/xen/xsm/xsm_policy.c
>> +++ b/xen/xsm/xsm_policy.c
>> @@ -8,7 +8,7 @@
>>   *  Contributors:
>>   *  Michael LeMay, <mdlemay@xxxxxxxxxxxxxx>
>>   *  George Coker, <gscoker@xxxxxxxxxxxxxx>
>> - *  
>> + *
>>   *  This program is free software; you can redistribute it and/or modify
>>   *  it under the terms of the GNU General Public License version 2,
>>   *  as published by the Free Software Foundation.
>> @@ -32,14 +32,21 @@
>>  #ifdef CONFIG_MULTIBOOT
>>  int __init xsm_multiboot_policy_init(
>>      unsigned long *module_map, const multiboot_info_t *mbi,
>> -    void **policy_buffer, size_t *policy_size)
>> +    const unsigned char **policy_buffer, size_t *policy_size)
>>  {
>>      int i;
>>      module_t *mod = (module_t *)__va(mbi->mods_addr);
>> -    int rc = 0;
>> +    int rc = -ENOENT;
>>      u32 *_policy_start;
>>      unsigned long _policy_len;
>>  
>> +#ifdef CONFIG_XSM_FLASK_POLICY
>> +    /* Initially set to builtin policy, overriden if boot module is found. 
>> */
>> +    *policy_buffer = xsm_flask_init_policy;
>> +    *policy_size = xsm_flask_init_policy_size;
>> +    rc = 0;
>> +#endif
> 
> Does
> 
> if ( IS_ENABLED(CONFIG_XSM_FLASK_POLICY) )
> {
>     ...
> }
> 
> compile properly?  You'll need to drop the ifdefary in xsm.h, but this
> would be a better approach (more compiler coverage in normal builds).
> 
> Same for the related hunk on the DT side.

Yes, I know this pattern is used elsewhere, so it should work here fine.

>> +
>>      /*
>>       * Try all modules and see whichever could be the binary policy.
>>       * Adjust module_map for the module that is the binary policy.
>> @@ -54,13 +61,14 @@ int __init xsm_multiboot_policy_init(
>>  
>>          if ( (xsm_magic_t)(*_policy_start) == XSM_MAGIC )
>>          {
>> -            *policy_buffer = _policy_start;
>> +            *policy_buffer = (unsigned char *)_policy_start;
> 
> The existing logic is horrible.  To start with, there's a buffer overrun
> for a module of fewer than 4 bytes.  (Same on the DT side.)
> 
> It would be slightly less horrible as
> 
> for ( ... )
> {
>     void *ptr;
> 
>     if ( !test_bit(i, module_map) || mod[i].mod_end < sizeof(xsm_header) )
>         continue;
> 
>     ptr = bootstrap_map(...);
> 
>     if ( memcmp(ptr, XSM_MAGIC, sizeof(XSM_MAGIC)) == 0 )
>     {
>         *policy_buffer = ptr;
>         *policy_len = mod[i].mod_end;
> 
>         ...
>         break;
>     }
> 
>     bootstrap_map(NULL);
> }
> 
> because at least this gets rid of the intermediate variables, the buffer
> overrun, and the awkward casting to find XSM_MAGIC.

Since you were kind enough to take the time to write out the fix, I can
incorporate.

> That said, it's still unclear what's going on, because has_xsm_magic()
> says the header is 16 bytes long, rather than 4 (assuming that it means
> uint32_t.  policydb_read() uses uint32_t).
> 
> Also, policydb_read() uses le32_to_cpu() so the multiboot/dt checks are
> wrong on big-endian systems.
> 
> Also also, policydb_read() really doesn't need to make a temporary
> memory allocation to check a fixed string of fixed length.  This is
> horrible.
> 
> I suspect we're getting into "in a subsequent patch" territory here.

Unfortunately the scope of what this series started out to solve, not to
walk all the boot modules when no policy file is needed, and what the
reviewers have been requesting be addressed is continually diverging.
Granted, with the technical debt currently present in XSM, I understand
why this is occurring. Unfortunately, subsequent patch here means, going
on to my list of things I would like to fix/work on in XSM.

v/r,
dps