Re: [PATCH v2 1/2] libxc: Don't write terminating NULL character to command string

[Boris Ostrovsky <boris.ostrovsky@oracle.com>]

On 01/05/2016 05:42 PM, Andrew Cooper wrote:
> On 05/01/2016 22:26, Boris Ostrovsky wrote:
>> When copying boot command string for HVMlite guests we explicitly write
>> '\0' at MAX_GUEST_CMDLINE offset. Unless the string is close to
>> MAX_GUEST_CMDLINE in length this write will end up in the wrong place,
>> beyond the end of the mapped range.
>>
>> Instead we should test string's length early and error out if it is too
>> long.
>>
>> Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
> MAX_GUEST_CMDLINE is an arbitrary and incorrect restriction.  It is
> sadly baked into the PV ABI, but I specifically want to avoid lumbering
> DMLite with the failings of PV.
>
> By the looks of it, the only bug is the use of MAX_GUEST_CMDLINE.  The
> xc_map_foreign_range() call already accounts for sufficient space to
> store the string when mapping guest memory.

Yes, I was also thinking about dropping it but ended up keeping it 
mostly because it didn't feel right to blindly use strcpy().

-boris


>
> I think you only need the 2nd hunk of this patch.
>
> ~Andrew
>
>> ---
>>   tools/libxc/xc_dom_x86.c |    8 ++++++--
>>   1 files changed, 6 insertions(+), 2 deletions(-)
>>
>> diff --git a/tools/libxc/xc_dom_x86.c b/tools/libxc/xc_dom_x86.c
>> index 3960875..b696149 100644
>> --- a/tools/libxc/xc_dom_x86.c
>> +++ b/tools/libxc/xc_dom_x86.c
>> @@ -647,6 +647,11 @@ static int alloc_magic_pages_hvm(struct xc_dom_image *dom)
>>           if ( dom->cmdline )
>>           {
>>               cmdline_size = ROUNDUP(strlen(dom->cmdline) + 1, 8);
>> +            if ( cmdline_size > MAX_GUEST_CMDLINE )
>> +            {
>> +                DOMPRINTF("Boot command line is too long");
>> +                goto error_out;
>> +            }
>>               start_info_size += cmdline_size;
>>   
>>           }
>> @@ -676,8 +681,7 @@ static int alloc_magic_pages_hvm(struct xc_dom_image *dom)
>>   
>>           if ( dom->cmdline )
>>           {
>> -            strncpy(cmdline, dom->cmdline, MAX_GUEST_CMDLINE);
>> -            cmdline[MAX_GUEST_CMDLINE - 1] = '\0';
>> +            strcpy(cmdline, dom->cmdline);
>>               start_info->cmdline_paddr = (seg.pfn << PAGE_SHIFT) +
>>                                   ((uintptr_t)cmdline - (uintptr_t)start_info);
>>           }