diff mbox series

x86/debug: fix page-overflow bug in dbg_rw_guest_mem

Message ID caba05850df644814d75d5de0574c62ce90e8789.1611971959.git.tamas@tklengyel.com (mailing list archive)
State New
Headers show
Series x86/debug: fix page-overflow bug in dbg_rw_guest_mem | expand

Commit Message

Tamas K Lengyel Jan. 30, 2021, 1:59 a.m. UTC
When using gdbsx dbg_rw_guest_mem is used to read/write guest memory. When the
buffer being accessed is on a page-boundary, the next page needs to be grabbed
to access the correct memory for the buffer's overflown parts. While
dbg_rw_guest_mem has logic to handle that, it broke with 229492e210a. Instead
of grabbing the next page the code right now is looping back to the
start of the first page. This results in errors like the following while trying
to use gdb with Linux' lx-dmesg:

[    0.114457] PM: hibernation: Registered nosave memory: [mem
0xfdfff000-0xffffffff]
[    0.114460] [mem 0x90000000-0xfbffffff] available for PCI demem 0
[    0.114462] f]f]
Python Exception <class 'ValueError'> embedded null character:
Error occurred in Python: embedded null character

Fixing this bug by taking the variable assignment outside the loop.

Signed-off-by: Tamas K Lengyel <tamas@tklengyel.com>
---
 xen/arch/x86/debug.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Comments

Andrew Cooper Jan. 30, 2021, 2:59 a.m. UTC | #1
On 30/01/2021 01:59, Tamas K Lengyel wrote:
> When using gdbsx dbg_rw_guest_mem is used to read/write guest memory. When the
> buffer being accessed is on a page-boundary, the next page needs to be grabbed
> to access the correct memory for the buffer's overflown parts. While
> dbg_rw_guest_mem has logic to handle that, it broke with 229492e210a. Instead
> of grabbing the next page the code right now is looping back to the
> start of the first page. This results in errors like the following while trying
> to use gdb with Linux' lx-dmesg:
>
> [    0.114457] PM: hibernation: Registered nosave memory: [mem
> 0xfdfff000-0xffffffff]
> [    0.114460] [mem 0x90000000-0xfbffffff] available for PCI demem 0
> [    0.114462] f]f]
> Python Exception <class 'ValueError'> embedded null character:
> Error occurred in Python: embedded null character
>
> Fixing this bug by taking the variable assignment outside the loop.
>
> Signed-off-by: Tamas K Lengyel <tamas@tklengyel.com>

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

Sorry for breaking this...
Jan Beulich Feb. 1, 2021, 9:37 a.m. UTC | #2
On 30.01.2021 03:59, Andrew Cooper wrote:
> On 30/01/2021 01:59, Tamas K Lengyel wrote:
>> When using gdbsx dbg_rw_guest_mem is used to read/write guest memory. When the
>> buffer being accessed is on a page-boundary, the next page needs to be grabbed
>> to access the correct memory for the buffer's overflown parts. While
>> dbg_rw_guest_mem has logic to handle that, it broke with 229492e210a. Instead
>> of grabbing the next page the code right now is looping back to the
>> start of the first page. This results in errors like the following while trying
>> to use gdb with Linux' lx-dmesg:
>>
>> [    0.114457] PM: hibernation: Registered nosave memory: [mem
>> 0xfdfff000-0xffffffff]
>> [    0.114460] [mem 0x90000000-0xfbffffff] available for PCI demem 0
>> [    0.114462] f]f]
>> Python Exception <class 'ValueError'> embedded null character:
>> Error occurred in Python: embedded null character
>>
>> Fixing this bug by taking the variable assignment outside the loop.
>>
>> Signed-off-by: Tamas K Lengyel <tamas@tklengyel.com>
> 
> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

I have to admit that I'm irritated: On January 14th I did submit
a patch ('x86/gdbsx: convert "user" to "guest" accesses') fixing this
as a side effect. I understand that one was taking care of more
issues here, but shouldn't that be preferred? Re-basing isn't going
to be overly difficult, but anyway.

Jan
Andrew Cooper Feb. 1, 2021, 11:44 a.m. UTC | #3
On 01/02/2021 09:37, Jan Beulich wrote:
> On 30.01.2021 03:59, Andrew Cooper wrote:
>> On 30/01/2021 01:59, Tamas K Lengyel wrote:
>>> When using gdbsx dbg_rw_guest_mem is used to read/write guest memory. When the
>>> buffer being accessed is on a page-boundary, the next page needs to be grabbed
>>> to access the correct memory for the buffer's overflown parts. While
>>> dbg_rw_guest_mem has logic to handle that, it broke with 229492e210a. Instead
>>> of grabbing the next page the code right now is looping back to the
>>> start of the first page. This results in errors like the following while trying
>>> to use gdb with Linux' lx-dmesg:
>>>
>>> [    0.114457] PM: hibernation: Registered nosave memory: [mem
>>> 0xfdfff000-0xffffffff]
>>> [    0.114460] [mem 0x90000000-0xfbffffff] available for PCI demem 0
>>> [    0.114462] f]f]
>>> Python Exception <class 'ValueError'> embedded null character:
>>> Error occurred in Python: embedded null character
>>>
>>> Fixing this bug by taking the variable assignment outside the loop.
>>>
>>> Signed-off-by: Tamas K Lengyel <tamas@tklengyel.com>
>> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
> I have to admit that I'm irritated: On January 14th I did submit
> a patch ('x86/gdbsx: convert "user" to "guest" accesses') fixing this
> as a side effect. I understand that one was taking care of more
> issues here, but shouldn't that be preferred? Re-basing isn't going
> to be overly difficult, but anyway.

I'm sorry.  That was sent during the period where I had no email access
(hence I wasn't aware of it - I've been focusing on 4.15 work and this
series wasn't pinged.), but it also isn't identified as a bugfix, or
suitable for backporting in that form.

I apologise for the extra work caused unintentionally, but I think this
is the correct way around WRT backports, is it not?

~Andrew
Jan Beulich Feb. 1, 2021, 12:29 p.m. UTC | #4
On 01.02.2021 12:44, Andrew Cooper wrote:
> On 01/02/2021 09:37, Jan Beulich wrote:
>> On 30.01.2021 03:59, Andrew Cooper wrote:
>>> On 30/01/2021 01:59, Tamas K Lengyel wrote:
>>>> When using gdbsx dbg_rw_guest_mem is used to read/write guest memory. When the
>>>> buffer being accessed is on a page-boundary, the next page needs to be grabbed
>>>> to access the correct memory for the buffer's overflown parts. While
>>>> dbg_rw_guest_mem has logic to handle that, it broke with 229492e210a. Instead
>>>> of grabbing the next page the code right now is looping back to the
>>>> start of the first page. This results in errors like the following while trying
>>>> to use gdb with Linux' lx-dmesg:
>>>>
>>>> [    0.114457] PM: hibernation: Registered nosave memory: [mem
>>>> 0xfdfff000-0xffffffff]
>>>> [    0.114460] [mem 0x90000000-0xfbffffff] available for PCI demem 0
>>>> [    0.114462] f]f]
>>>> Python Exception <class 'ValueError'> embedded null character:
>>>> Error occurred in Python: embedded null character
>>>>
>>>> Fixing this bug by taking the variable assignment outside the loop.
>>>>
>>>> Signed-off-by: Tamas K Lengyel <tamas@tklengyel.com>
>>> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
>> I have to admit that I'm irritated: On January 14th I did submit
>> a patch ('x86/gdbsx: convert "user" to "guest" accesses') fixing this
>> as a side effect. I understand that one was taking care of more
>> issues here, but shouldn't that be preferred? Re-basing isn't going
>> to be overly difficult, but anyway.
> 
> I'm sorry.  That was sent during the period where I had no email access
> (hence I wasn't aware of it - I've been focusing on 4.15 work and this
> series wasn't pinged.),

Oh, so you had actually lost emails, rather than (as I did
understand so far) only getting them in a very delayed fashion?

Anyway, the first part of that series having been pretty close
to getting an XSA, I thought you were well aware that at least
that part is very clearly intended for 4.15. (I also did
mention it to you last week on irc, when you asked what wants
specifically looking at for 4.15.) Plus, besides the bringing
up of the topic on the last two or three community calls, over
all of January I've specifically avoided pinging _any_ of the
many patches I have pending, to avoid giving you the feel of
even more pressure.

> but it also isn't identified as a bugfix, or
> suitable for backporting in that form.
> 
> I apologise for the extra work caused unintentionally, but I think this
> is the correct way around WRT backports, is it not?

It didn't occur to me that there could be a consideration of
backporting here. But yes, if so wanted, maybe the split is
helpful. Otoh then the full change could as well be taken,
to stop the abuse of "user" accesses also in the stable trees.

Jan
Tamas K Lengyel Feb. 1, 2021, 4:36 p.m. UTC | #5
On Mon, Feb 1, 2021 at 7:29 AM Jan Beulich <jbeulich@suse.com> wrote:
>
> On 01.02.2021 12:44, Andrew Cooper wrote:
> > On 01/02/2021 09:37, Jan Beulich wrote:
> >> On 30.01.2021 03:59, Andrew Cooper wrote:
> >>> On 30/01/2021 01:59, Tamas K Lengyel wrote:
> >>>> When using gdbsx dbg_rw_guest_mem is used to read/write guest memory. When the
> >>>> buffer being accessed is on a page-boundary, the next page needs to be grabbed
> >>>> to access the correct memory for the buffer's overflown parts. While
> >>>> dbg_rw_guest_mem has logic to handle that, it broke with 229492e210a. Instead
> >>>> of grabbing the next page the code right now is looping back to the
> >>>> start of the first page. This results in errors like the following while trying
> >>>> to use gdb with Linux' lx-dmesg:
> >>>>
> >>>> [    0.114457] PM: hibernation: Registered nosave memory: [mem
> >>>> 0xfdfff000-0xffffffff]
> >>>> [    0.114460] [mem 0x90000000-0xfbffffff] available for PCI demem 0
> >>>> [    0.114462] f]f]
> >>>> Python Exception <class 'ValueError'> embedded null character:
> >>>> Error occurred in Python: embedded null character
> >>>>
> >>>> Fixing this bug by taking the variable assignment outside the loop.
> >>>>
> >>>> Signed-off-by: Tamas K Lengyel <tamas@tklengyel.com>
> >>> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
> >> I have to admit that I'm irritated: On January 14th I did submit
> >> a patch ('x86/gdbsx: convert "user" to "guest" accesses') fixing this
> >> as a side effect. I understand that one was taking care of more
> >> issues here, but shouldn't that be preferred? Re-basing isn't going
> >> to be overly difficult, but anyway.
> >
> > I'm sorry.  That was sent during the period where I had no email access
> > (hence I wasn't aware of it - I've been focusing on 4.15 work and this
> > series wasn't pinged.),
>
> Oh, so you had actually lost emails, rather than (as I did
> understand so far) only getting them in a very delayed fashion?
>
> Anyway, the first part of that series having been pretty close
> to getting an XSA, I thought you were well aware that at least
> that part is very clearly intended for 4.15. (I also did
> mention it to you last week on irc, when you asked what wants
> specifically looking at for 4.15.) Plus, besides the bringing
> up of the topic on the last two or three community calls, over
> all of January I've specifically avoided pinging _any_ of the
> many patches I have pending, to avoid giving you the feel of
> even more pressure.
>
> > but it also isn't identified as a bugfix, or
> > suitable for backporting in that form.
> >
> > I apologise for the extra work caused unintentionally, but I think this
> > is the correct way around WRT backports, is it not?
>
> It didn't occur to me that there could be a consideration of
> backporting here. But yes, if so wanted, maybe the split is
> helpful. Otoh then the full change could as well be taken,
> to stop the abuse of "user" accesses also in the stable trees.

IMHO this should be backported cause it breaks use of gdbsx for all
affected releases.

Tamas
diff mbox series

Patch

diff --git a/xen/arch/x86/debug.c b/xen/arch/x86/debug.c
index 4356039ed2..f32d4b0bcc 100644
--- a/xen/arch/x86/debug.c
+++ b/xen/arch/x86/debug.c
@@ -112,10 +112,11 @@  static unsigned int dbg_rw_guest_mem(struct domain *dp, void * __user gaddr,
                                      void * __user buf, unsigned int len,
                                      bool toaddr, uint64_t pgd3)
 {
+    unsigned long addr = (unsigned long)gaddr;
+
     while ( len > 0 )
     {
         char *va;
-        unsigned long addr = (unsigned long)gaddr;
         mfn_t mfn;
         gfn_t gfn = INVALID_GFN;
         unsigned long pagecnt;