[PATCH for-4.11 0/2] libxc/x86: fix a couple of bugs

[PATCH for-4.11 0/2] libxc/x86: fix a couple of bugs

[Roger Pau Monne]

Hello,

Following two patches fix two bugs related to the mapping and handling
of the hvm_start_info.

Thanks, Roger.

Roger Pau Monne (2):
  libxc/x86: fix mapping of the start_info area
  libxc/x86: do not unconditionally set the module cmdline address

 tools/libxc/xc_dom_x86.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

-- 
2.16.2


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

[PATCH for-4.11 1/2] libxc/x86: fix mapping of the start_info area

[Roger Pau Monne]

The start_info size calculated in bootlate_hvm is wrong. It should use
HVMLOADER_MODULE_MAX_COUNT instead of dom->num_modules and it doesn't
take into account the size of the modules command line.

This is not a problem so far because the actually used amount of
memory doesn't cross a page boundary, and so no page-fault is
triggered.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Wei Liu <wei.liu2@citrix.com>
---
 tools/libxc/xc_dom_x86.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/tools/libxc/xc_dom_x86.c b/tools/libxc/xc_dom_x86.c
index 0b65dab4bc..e29c666b89 100644
--- a/tools/libxc/xc_dom_x86.c
+++ b/tools/libxc/xc_dom_x86.c
@@ -1671,7 +1671,11 @@ static int bootlate_hvm(struct xc_dom_image *dom)
     unsigned int i;
 
     start_info_size = sizeof(*start_info) + dom->cmdline_size;
-    start_info_size += sizeof(struct hvm_modlist_entry) * dom->num_modules;
+    start_info_size += sizeof(struct hvm_modlist_entry) *
+                       HVMLOADER_MODULE_MAX_COUNT;
+    start_info_size += HVMLOADER_MODULE_CMDLINE_SIZE *
+                       HVMLOADER_MODULE_MAX_COUNT;
+
 
     if ( start_info_size >
          dom->start_info_seg.pages << XC_DOM_PAGE_SHIFT(dom) )
-- 
2.16.2


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

[PATCH for-4.11 2/2] libxc/x86: do not unconditionally set the module cmdline address

[Roger Pau Monne]

This will lead to writing a wrong module command line physical memory
address if no command line is actually provided.

This hasn't caused problems so far because hvmloader is the only
consumer of the modules command line, and it's unconditionally set
in that case.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Wei Liu <wei.liu2@citrix.com>
---
 tools/libxc/xc_dom_x86.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/tools/libxc/xc_dom_x86.c b/tools/libxc/xc_dom_x86.c
index e29c666b89..27dad89906 100644
--- a/tools/libxc/xc_dom_x86.c
+++ b/tools/libxc/xc_dom_x86.c
@@ -1653,11 +1653,10 @@ static void add_module_to_list(struct xc_dom_image *dom,
                < HVMLOADER_MODULE_CMDLINE_SIZE);
         strncpy(modules_cmdline_start + HVMLOADER_MODULE_CMDLINE_SIZE * index,
                 cmdline, HVMLOADER_MODULE_CMDLINE_SIZE);
+        modlist[index].cmdline_paddr = modules_cmdline_paddr +
+                                       HVMLOADER_MODULE_CMDLINE_SIZE * index;
     }
 
-    modlist[index].cmdline_paddr =
-        modules_cmdline_paddr + HVMLOADER_MODULE_CMDLINE_SIZE * index;
-
     start_info->nr_modules++;
 }
 
-- 
2.16.2


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

Re: [PATCH for-4.11 1/2] libxc/x86: fix mapping of the start_info area

[Wei Liu]

On Wed, Mar 21, 2018 at 02:42:10PM +0000, Roger Pau Monne wrote:
> The start_info size calculated in bootlate_hvm is wrong. It should use
> HVMLOADER_MODULE_MAX_COUNT instead of dom->num_modules and it doesn't
> take into account the size of the modules command line.
> 
> This is not a problem so far because the actually used amount of
> memory doesn't cross a page boundary, and so no page-fault is
> triggered.

I get the cmdline bit.

What does it need to be HVMLOADER_MODULE_MAX_COUNT? Isn't better to just
map what we need here?

Wei.

> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> ---
> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> Cc: Wei Liu <wei.liu2@citrix.com>
> ---
>  tools/libxc/xc_dom_x86.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/libxc/xc_dom_x86.c b/tools/libxc/xc_dom_x86.c
> index 0b65dab4bc..e29c666b89 100644
> --- a/tools/libxc/xc_dom_x86.c
> +++ b/tools/libxc/xc_dom_x86.c
> @@ -1671,7 +1671,11 @@ static int bootlate_hvm(struct xc_dom_image *dom)
>      unsigned int i;
>  
>      start_info_size = sizeof(*start_info) + dom->cmdline_size;
> -    start_info_size += sizeof(struct hvm_modlist_entry) * dom->num_modules;
> +    start_info_size += sizeof(struct hvm_modlist_entry) *
> +                       HVMLOADER_MODULE_MAX_COUNT;
> +    start_info_size += HVMLOADER_MODULE_CMDLINE_SIZE *
> +                       HVMLOADER_MODULE_MAX_COUNT;
> +
>  
>      if ( start_info_size >
>           dom->start_info_seg.pages << XC_DOM_PAGE_SHIFT(dom) )
> -- 
> 2.16.2
> 

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

Re: [PATCH for-4.11 2/2] libxc/x86: do not unconditionally set the module cmdline address

[Wei Liu]

On Wed, Mar 21, 2018 at 02:42:11PM +0000, Roger Pau Monne wrote:
> This will lead to writing a wrong module command line physical memory
> address if no command line is actually provided.
> 
> This hasn't caused problems so far because hvmloader is the only
> consumer of the modules command line, and it's unconditionally set
> in that case.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

Acked-by: Wei Liu <wei.liu2@citrix.com>

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

Re: [PATCH for-4.11 1/2] libxc/x86: fix mapping of the start_info area

[Roger Pau Monné]

On Wed, Mar 21, 2018 at 06:09:57PM +0000, Wei Liu wrote:
> On Wed, Mar 21, 2018 at 02:42:10PM +0000, Roger Pau Monne wrote:
> > The start_info size calculated in bootlate_hvm is wrong. It should use
> > HVMLOADER_MODULE_MAX_COUNT instead of dom->num_modules and it doesn't
> > take into account the size of the modules command line.
> > 
> > This is not a problem so far because the actually used amount of
> > memory doesn't cross a page boundary, and so no page-fault is
> > triggered.
> 
> I get the cmdline bit.
> 
> What does it need to be HVMLOADER_MODULE_MAX_COUNT? Isn't better to just
> map what we need here?

Because the position of the modules command line is:

modlist_paddr + sizeof(struct hvm_modlist_entry) * HVMLOADER_MODULE_MAX_COUNT;

(This is from add_module_to_list).

So if dom->num_modules < HVMLOADER_MODULE_MAX_COUNT the mapped region
is smaller that what we might end up using.

I'm not sure why HVMLOADER_MODULE_MAX_COUNT is used when allocating
memory (in alloc_magic_pages_hvm) instead of the actual number of
modules (dom->num_modules), but the proposed change seems to be the
easier way to fix the mapping issue.

I've CC'ed the original authors of this code in another thread, but
got no response.

Thanks, Roger.

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

Re: [PATCH for-4.11 1/2] libxc/x86: fix mapping of the start_info area

[Roger Pau Monné]

Ping?

On Thu, Mar 22, 2018 at 09:10:03AM +0000, Roger Pau Monné wrote:
> On Wed, Mar 21, 2018 at 06:09:57PM +0000, Wei Liu wrote:
> > On Wed, Mar 21, 2018 at 02:42:10PM +0000, Roger Pau Monne wrote:
> > > The start_info size calculated in bootlate_hvm is wrong. It should use
> > > HVMLOADER_MODULE_MAX_COUNT instead of dom->num_modules and it doesn't
> > > take into account the size of the modules command line.
> > > 
> > > This is not a problem so far because the actually used amount of
> > > memory doesn't cross a page boundary, and so no page-fault is
> > > triggered.
> > 
> > I get the cmdline bit.
> > 
> > What does it need to be HVMLOADER_MODULE_MAX_COUNT? Isn't better to just
> > map what we need here?
> 
> Because the position of the modules command line is:
> 
> modlist_paddr + sizeof(struct hvm_modlist_entry) * HVMLOADER_MODULE_MAX_COUNT;
> 
> (This is from add_module_to_list).
> 
> So if dom->num_modules < HVMLOADER_MODULE_MAX_COUNT the mapped region
> is smaller that what we might end up using.
> 
> I'm not sure why HVMLOADER_MODULE_MAX_COUNT is used when allocating
> memory (in alloc_magic_pages_hvm) instead of the actual number of
> modules (dom->num_modules), but the proposed change seems to be the
> easier way to fix the mapping issue.
> 
> I've CC'ed the original authors of this code in another thread, but
> got no response.
> 
> Thanks, Roger.
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xenproject.org
> https://lists.xenproject.org/mailman/listinfo/xen-devel

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

Re: [PATCH for-4.11 1/2] libxc/x86: fix mapping of the start_info area

[Wei Liu]

On Thu, Mar 22, 2018 at 09:10:03AM +0000, Roger Pau Monné wrote:
> On Wed, Mar 21, 2018 at 06:09:57PM +0000, Wei Liu wrote:
> > On Wed, Mar 21, 2018 at 02:42:10PM +0000, Roger Pau Monne wrote:
> > > The start_info size calculated in bootlate_hvm is wrong. It should use
> > > HVMLOADER_MODULE_MAX_COUNT instead of dom->num_modules and it doesn't
> > > take into account the size of the modules command line.
> > > 
> > > This is not a problem so far because the actually used amount of
> > > memory doesn't cross a page boundary, and so no page-fault is
> > > triggered.
> > 
> > I get the cmdline bit.
> > 
> > What does it need to be HVMLOADER_MODULE_MAX_COUNT? Isn't better to just
> > map what we need here?
> 
> Because the position of the modules command line is:
> 
> modlist_paddr + sizeof(struct hvm_modlist_entry) * HVMLOADER_MODULE_MAX_COUNT;
> 
> (This is from add_module_to_list).
> 
> So if dom->num_modules < HVMLOADER_MODULE_MAX_COUNT the mapped region
> is smaller that what we might end up using.
> 
> I'm not sure why HVMLOADER_MODULE_MAX_COUNT is used when allocating
> memory (in alloc_magic_pages_hvm) instead of the actual number of
> modules (dom->num_modules), but the proposed change seems to be the
> easier way to fix the mapping issue.
> 

This patch is correct, in the sense that it replicates the logic from
alloc_magic_pages_hvm to bootlate_hvm. However, I don't think
bootlate_hvm is in the business of calculating the size once more. This
is bound to fail in the future.

Instead, you can stash the size to dom once the calculation in
alloc_magic_pages_hvm is done, and then use it in bootlate_hvm. This is
the least fragile way I can think of.

Does this make sense?

Wei.

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

Re: [PATCH for-4.11 1/2] libxc/x86: fix mapping of the start_info area

[Roger Pau Monné]

On Wed, Mar 28, 2018 at 12:08:07PM +0100, Wei Liu wrote:
> On Thu, Mar 22, 2018 at 09:10:03AM +0000, Roger Pau Monné wrote:
> > On Wed, Mar 21, 2018 at 06:09:57PM +0000, Wei Liu wrote:
> > > On Wed, Mar 21, 2018 at 02:42:10PM +0000, Roger Pau Monne wrote:
> > > > The start_info size calculated in bootlate_hvm is wrong. It should use
> > > > HVMLOADER_MODULE_MAX_COUNT instead of dom->num_modules and it doesn't
> > > > take into account the size of the modules command line.
> > > > 
> > > > This is not a problem so far because the actually used amount of
> > > > memory doesn't cross a page boundary, and so no page-fault is
> > > > triggered.
> > > 
> > > I get the cmdline bit.
> > > 
> > > What does it need to be HVMLOADER_MODULE_MAX_COUNT? Isn't better to just
> > > map what we need here?
> > 
> > Because the position of the modules command line is:
> > 
> > modlist_paddr + sizeof(struct hvm_modlist_entry) * HVMLOADER_MODULE_MAX_COUNT;
> > 
> > (This is from add_module_to_list).
> > 
> > So if dom->num_modules < HVMLOADER_MODULE_MAX_COUNT the mapped region
> > is smaller that what we might end up using.
> > 
> > I'm not sure why HVMLOADER_MODULE_MAX_COUNT is used when allocating
> > memory (in alloc_magic_pages_hvm) instead of the actual number of
> > modules (dom->num_modules), but the proposed change seems to be the
> > easier way to fix the mapping issue.
> > 
> 
> This patch is correct, in the sense that it replicates the logic from
> alloc_magic_pages_hvm to bootlate_hvm. However, I don't think
> bootlate_hvm is in the business of calculating the size once more. This
> is bound to fail in the future.

Agree, the calculation now is fairly simple, yet we have already
failed to replicate it properly.

> Instead, you can stash the size to dom once the calculation in
> alloc_magic_pages_hvm is done, and then use it in bootlate_hvm. This is
> the least fragile way I can think of.

Ack, I think this is correct, and more robust future-wise.

Thanks, Roger.

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