Re: [Xen-devel] [PATCH] libxl/libxl_arm_acpi.c: fix 'memcpy' forming offset out of the bounds

[Wei Liu <wei.liu2@xxxxxxxxxx>]

On Thu, Jul 26, 2018 at 09:09:35AM +0100, Andrew Cooper wrote:
> On 26/07/2018 08:46, Wei Liu wrote:
> > On Wed, Jul 25, 2018 at 04:44:25PM -0700, Manjukumar Matha wrote:
> >> gcc-8.1 complains:
> >>
> >> libxl_arm_acpi.c:208:5: error: 'memcpy' forming offset [5, 6] is out of
> >> the bounds [0, 4] [-Werror=array-bounds]
> >>      memcpy(h->oem_id, ACPI_OEM_ID, sizeof(h->oem_id));
> >>      ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> >> libxl_arm_acpi.c:209:5: error: 'memcpy' forming offset [5, 8] is out of
> >> the bounds [0, 4] [-Werror=array-bounds]
> >>      memcpy(h->oem_table_id, ACPI_OEM_TABLE_ID,
> >> sizeof(h->oem_table_id));
> >>      ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> >> libxl_arm_acpi.c:211:5: error: 'memcpy' forming offset 4 is out of the
> >> bounds [0, 3] [-Werror=array-bounds]
> >>      memcpy(h->asl_compiler_id, ACPI_ASL_COMPILER_ID,
> >>      ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> >>             sizeof(h->asl_compiler_id));
> >>             ~~~~~~~~~~~~~~~~~~~~~~~~~~~
> >> libxl_arm_acpi.c:193:5: error: 'memcpy' forming offset [5, 6] is out of
> >> the bounds [0, 4] [-Werror=array-bounds]
> >>      memcpy(rsdp->oem_id, ACPI_OEM_ID, sizeof(rsdp->oem_id));
> >>      ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> >>
> >> This patch fixes the above errors.
> >>
> >> Signed-off-by: Manjukumar Matha <manjukumar.harthikote-matha@xxxxxxxxxx>
> >> ---
> >>  tools/libxl/libxl_arm_acpi.c | 9 ++++-----
> >>  1 file changed, 4 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/tools/libxl/libxl_arm_acpi.c b/tools/libxl/libxl_arm_acpi.c
> >> index 636f724..9b300f3 100644
> >> --- a/tools/libxl/libxl_arm_acpi.c
> >> +++ b/tools/libxl/libxl_arm_acpi.c
> >> @@ -190,7 +190,7 @@ static void make_acpi_rsdp(libxl__gc *gc, struct 
> >> xc_dom_image *dom,
> >>      struct acpi_table_rsdp *rsdp = (void *)dom->acpi_modules[0].data + 
> >> offset;
> >>  
> >>      memcpy(rsdp->signature, "RSD PTR ", sizeof(rsdp->signature));
> >> -    memcpy(rsdp->oem_id, ACPI_OEM_ID, sizeof(rsdp->oem_id));
> >> +    memcpy(rsdp->oem_id, ACPI_OEM_ID, sizeof(ACPI_OEM_ID));
> > The code should pick the smaller size of the two as the copy length.
> > Or, since this is string, use strncpy with size of the destination
> > array.
> 
> Urgh - this is mess.
> 
> The behaviour you want is something like that of strncpy(), because it
> will add trailing NUL's to the destination if the size exceeds the
> length of src.
> 
> The problem with this is that a) you want space rather than NUL padding,
> b) Coverity/compilers are going to complain bitterly (because of the use
> of str*() functions) that you end up with a non-NUL terminated.
> 
> Perhaps the best option is to write a new helper which has strncpy()'s
> behaviour except for having space padding, and which Coverity/compilers
> won't apply str*() sanity checking to.

This works for me too.

The ideal location of that function should be in libacpi.

Wei.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel