[Skiboot] [PATCH 2/5] nvram: ibm,skiboot NUL terminator check

[Oliver O'Halloran]

On Mon, Aug 8, 2016 at 6:00 PM, Joel Stanley <joel at jms.id.au> wrote:
> On Mon, Aug 8, 2016 at 12:28 PM, Oliver O'Halloran <oohall at gmail.com> wrote:
>> NVRAM configuration strings are required to be NUL terminated and unused
>> data bytes in the partition should be set to NUL. Badly behaved system
>> software may not do this so same sanity checking is required. Ensuring
>> that the final data byte in a partition is a NUL should be sufficent.
>
> Spelling (sufficient).

fixed

>
>
>>
>> Signed-off-by: Oliver O'Halloran <oohall at gmail.com>
>> ---
>>  core/nvram-format.c | 14 ++++++++++++++
>>  1 file changed, 14 insertions(+)
>>
>> diff --git a/core/nvram-format.c b/core/nvram-format.c
>> index a81663ceb35f..be8d77f8394b 100644
>> --- a/core/nvram-format.c
>> +++ b/core/nvram-format.c
>> @@ -155,6 +155,20 @@ int nvram_check(void *nvram_image, const uint32_t nvram_size)
>>                 prerror("NVRAM: Skiboot private partition "
>>                         "not found !\n");
>>                 goto failed;
>> +       } else {
>> +               /*
>> +                * The OF NVRAM format requires config strings to be NUL
>> +                * terminated and unused memory to be set ot zero. Well behaved
>
> Typo

fixed

>
>> +                * software should ensure this is done for us, but we should
>> +                * always check.
>> +                */
>> +               const char *c = (const char *) skiboot_part_hdr +
>> +                       skiboot_part_hdr->len * 16 - 1;
>
> c is going to point to the last byte of the partition? We could call
> it 'last_byte'.

That's a better name.

> Is ->len user controlled? Do we need to make sure it's not pointing
> past the end of something?

It is user controlled, but it's validated earlier in that function
while iterating through the nvram partitions.

>
>> +
>> +               if (*c != 0) {
>> +                       prerror("NVRAM: Skiboot private partition is not NUL terminated");
>> +                       goto failed;
>> +               }
>>         }
>>
>>         prlog(PR_INFO, "NVRAM: Layout appears sane\n");
>> --
>> 2.5.5
>>
>> _______________________________________________
>> Skiboot mailing list
>> Skiboot at lists.ozlabs.org
>> https://lists.ozlabs.org/listinfo/skiboot