All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v2] virtio_mem: fix panic on mb_states indexing overflow
@ 2021-12-20 13:18 Eric Ren
  2021-12-20 14:15 ` David Hildenbrand
  0 siblings, 1 reply; 5+ messages in thread
From: Eric Ren @ 2021-12-20 13:18 UTC (permalink / raw)
  To: david, linux-mm; +Cc: linux-kernel, vbabka, ziy, renzhengeek

`mb_id` is unsigned integer, which is used to index
`mb_states` array in reverse order. `mb_id` can decrease
to `0UL - 1` that is a very large number, causing invalid
address access.

The calltrace is like below:
```
[  286.344977] BUG: unable to handle page fault for address: ffffa95180cf8fff
[  286.345800] #PF: supervisor read access in kernel mode
[  286.346738] #PF: error_code(0x0000) - not-present page
[  286.347440] PGD 1000067 P4D 1000067 PUD 138c067 PMD 1840435067 PTE 0
[  286.348156] Oops: 0000 [#1] SMP PTI                                                        [  286.348556] CPU: 1 PID: 122 Comm: kworker/1:2 Tainted: G           OE
...
[  286.350740] Workqueue: events_freezable virtio_mem_run_wq [virtio_mem]
[  286.351605] RIP: 0010:virtio_mem_unplug_request+0x418/0x890 [virtio_mem]
[  286.352519] Code: 0f 87 fc 00 00 00 4a 63 54 ac 30 48 83 bc d5 f8 00 00 00 00 48 89 d0 0f 8
4 e5 00 00 00 48 8b b5 38 01 00 00 4c 89 e2 48 29 ca <0f> b6 34 16 39 c6 75 c7 40 80 fe 02 0f
82 a4 01 00 00 40 80 fe 03
[  286.355030] RSP: 0018:ffffa95181c4bd50 EFLAGS: 00010286
[  286.355737] RAX: 0000000000000005 RBX: 0000000000006100 RCX: 0000000000000000
[  286.356752] RDX: ffffffffffffffff RSI: ffffa95180cf9000 RDI: ffff8e5dc393b348
[  286.357649] RBP: ffff8e5dc393b200 R08: ffff8e463cd2b610 R09: 0000000000000021
[  286.358627] R10: ffffa95181c4bcd0 R11: ffffa95181c4baa0 R12: ffffffffffffffff
[  286.359617] R13: 0000000000000003 R14: ffff8e5dc393b348 R15: 00000000fffffff0
[  286.360503] FS:  0000000000000000(0000) GS:ffff8e463cd00000(0000) knlGS:0000000000000000
[  286.361532] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[  286.362229] CR2: ffffa95180cf8fff CR3: 0000001846234000 CR4: 00000000000006e0
[  286.363168] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
[  286.364162] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
[  286.365054] Call Trace:
[  286.365431]  ? virtio_mem_run_wq+0x5a4/0x870 [virtio_mem]
[  286.366132]  ? __schedule+0x4b3/0x800
[  286.366547]  ? process_one_work+0x18b/0x350
[  286.367041]  ? worker_thread+0x4f/0x3a0
[  286.367675]  ? rescuer_thread+0x350/0x350
[  286.368234]  ? kthread+0xfa/0x130
[  286.368605]  ? kthread_create_worker_on_cpu+0x70/0x70
[  286.369155]  ? ret_from_fork+0x1f/0x30
```

Fixes by also checking its up boundary.

Signed-off-by: Eric Ren <renzhengeek@gmail.com>
--
v2: Correct the SOB
    Use _vm and remove duplicated spaces
---
 drivers/virtio/virtio_mem.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/virtio/virtio_mem.c b/drivers/virtio/virtio_mem.c
index 96e5a8782769..f1ba0dadd47a 100644
--- a/drivers/virtio/virtio_mem.c
+++ b/drivers/virtio/virtio_mem.c
@@ -486,7 +486,9 @@ static int virtio_mem_sbm_mb_states_prepare_next_mb(struct virtio_mem *vm)
 
 #define virtio_mem_sbm_for_each_mb_rev(_vm, _mb_id, _state) \
 	for (_mb_id = _vm->sbm.next_mb_id - 1; \
-	     _mb_id >= _vm->sbm.first_mb_id && _vm->sbm.mb_count[_state]; \
+	     _mb_id >= _vm->sbm.first_mb_id && \
+	     _mb_id < _vm->sbm.next_mb_id && \
+	     _vm->sbm.mb_count[_state]; \
 	     _mb_id--) \
 		if (virtio_mem_sbm_get_mb_state(_vm, _mb_id) == _state)
 
-- 
2.30.1 (Apple Git-130)


^ permalink raw reply related	[flat|nested] 5+ messages in thread
* Re: [PATCH v2] virtio_mem: fix panic on mb_states indexing overflow
  2021-12-20 13:18 [PATCH v2] virtio_mem: fix panic on mb_states indexing overflow Eric Ren
@ 2021-12-20 14:15 ` David Hildenbrand
  2021-12-21 12:39   ` Eric Ren
  0 siblings, 1 reply; 5+ messages in thread
From: David Hildenbrand @ 2021-12-20 14:15 UTC (permalink / raw)
  To: Eric Ren, linux-mm; +Cc: linux-kernel, vbabka, ziy

On 20.12.21 14:18, Eric Ren wrote:
> `mb_id` is unsigned integer, which is used to index
> `mb_states` array in reverse order. `mb_id` can decrease
> to `0UL - 1` that is a very large number, causing invalid
> address access.

Hi Eric,

thanks for your report!

The only way I can see this happening would be if

next_mb_id == 0, in which case we would get

	_mb_id = _vm->sbm.next_mb_id - 1
	-> _mb_id = 0 - 1 = -1ULL

Otherwise we should always stay above _vm->sbm.first_mb_id.

Am I correct or am I missing something?


But in that case we would have to have
	vm->sbm.first_mb_id == 0

and consequently the start address of the device would have to be at
address 0.

... but that does sound very weird, especially on x86_64?

Do you have details about the device layout / position in guest physical
address space?

virtio_mem_bbm_for_each_bb_rev() would need a similar fix.


> 
> The calltrace is like below:
> ```
> [  286.344977] BUG: unable to handle page fault for address: ffffa95180cf8fff
> [  286.345800] #PF: supervisor read access in kernel mode
> [  286.346738] #PF: error_code(0x0000) - not-present page
> [  286.347440] PGD 1000067 P4D 1000067 PUD 138c067 PMD 1840435067 PTE 0
> [  286.348156] Oops: 0000 [#1] SMP PTI                                                        [  286.348556] CPU: 1 PID: 122 Comm: kworker/1:2 Tainted: G           OE
> ...
> [  286.350740] Workqueue: events_freezable virtio_mem_run_wq [virtio_mem]
> [  286.351605] RIP: 0010:virtio_mem_unplug_request+0x418/0x890 [virtio_mem]
> [  286.352519] Code: 0f 87 fc 00 00 00 4a 63 54 ac 30 48 83 bc d5 f8 00 00 00 00 48 89 d0 0f 8
> 4 e5 00 00 00 48 8b b5 38 01 00 00 4c 89 e2 48 29 ca <0f> b6 34 16 39 c6 75 c7 40 80 fe 02 0f
> 82 a4 01 00 00 40 80 fe 03
> [  286.355030] RSP: 0018:ffffa95181c4bd50 EFLAGS: 00010286
> [  286.355737] RAX: 0000000000000005 RBX: 0000000000006100 RCX: 0000000000000000
> [  286.356752] RDX: ffffffffffffffff RSI: ffffa95180cf9000 RDI: ffff8e5dc393b348
> [  286.357649] RBP: ffff8e5dc393b200 R08: ffff8e463cd2b610 R09: 0000000000000021
> [  286.358627] R10: ffffa95181c4bcd0 R11: ffffa95181c4baa0 R12: ffffffffffffffff
> [  286.359617] R13: 0000000000000003 R14: ffff8e5dc393b348 R15: 00000000fffffff0
> [  286.360503] FS:  0000000000000000(0000) GS:ffff8e463cd00000(0000) knlGS:0000000000000000
> [  286.361532] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [  286.362229] CR2: ffffa95180cf8fff CR3: 0000001846234000 CR4: 00000000000006e0
> [  286.363168] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> [  286.364162] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
> [  286.365054] Call Trace:
> [  286.365431]  ? virtio_mem_run_wq+0x5a4/0x870 [virtio_mem]
> [  286.366132]  ? __schedule+0x4b3/0x800
> [  286.366547]  ? process_one_work+0x18b/0x350
> [  286.367041]  ? worker_thread+0x4f/0x3a0
> [  286.367675]  ? rescuer_thread+0x350/0x350
> [  286.368234]  ? kthread+0xfa/0x130
> [  286.368605]  ? kthread_create_worker_on_cpu+0x70/0x70
> [  286.369155]  ? ret_from_fork+0x1f/0x30
> ```
> 
> Fixes by also checking its up boundary.
> 
> Signed-off-by: Eric Ren <renzhengeek@gmail.com>
> --
> v2: Correct the SOB
>     Use _vm and remove duplicated spaces
> ---
>  drivers/virtio/virtio_mem.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/virtio/virtio_mem.c b/drivers/virtio/virtio_mem.c
> index 96e5a8782769..f1ba0dadd47a 100644
> --- a/drivers/virtio/virtio_mem.c
> +++ b/drivers/virtio/virtio_mem.c
> @@ -486,7 +486,9 @@ static int virtio_mem_sbm_mb_states_prepare_next_mb(struct virtio_mem *vm)
>  
>  #define virtio_mem_sbm_for_each_mb_rev(_vm, _mb_id, _state) \
>  	for (_mb_id = _vm->sbm.next_mb_id - 1; \
> -	     _mb_id >= _vm->sbm.first_mb_id && _vm->sbm.mb_count[_state]; \
> +	     _mb_id >= _vm->sbm.first_mb_id && \
> +	     _mb_id < _vm->sbm.next_mb_id && \
> +	     _vm->sbm.mb_count[_state]; \
>  	     _mb_id--) \
>  		if (virtio_mem_sbm_get_mb_state(_vm, _mb_id) == _state)
>  


-- 
Thanks,

David / dhildenb


^ permalink raw reply	[flat|nested] 5+ messages in thread
* Re: [PATCH v2] virtio_mem: fix panic on mb_states indexing overflow
  2021-12-20 14:15 ` David Hildenbrand
@ 2021-12-21 12:39   ` Eric Ren
  2021-12-21 13:53     ` David Hildenbrand
  0 siblings, 1 reply; 5+ messages in thread
From: Eric Ren @ 2021-12-21 12:39 UTC (permalink / raw)
  To: David Hildenbrand, linux-mm; +Cc: linux-kernel, vbabka, ziy

Hi,

在 2021/12/20 22:15, David Hildenbrand 写道:
> On 20.12.21 14:18, Eric Ren wrote:
>> `mb_id` is unsigned integer, which is used to index
>> `mb_states` array in reverse order. `mb_id` can decrease
>> to `0UL - 1` that is a very large number, causing invalid
>> address access.
> Hi Eric,
>
> thanks for your report!
>
> The only way I can see this happening would be if
>
> next_mb_id == 0, in which case we would get
>
> 	_mb_id = _vm->sbm.next_mb_id - 1
> 	-> _mb_id = 0 - 1 = -1ULL
In the for loop,  eg.

_vm->sbm.next_mb_id == 756
initial _mb_id = 755
_vm->sbm.first_mb_id == 0
_vm->sbm.mb_count[_state] == 1

_mb_id-- will decrease to 0, * at this time *, the condition below in loop is true:
	_mb_id >= _vm->sbm.first_mb_id && _vm->sbm.mb_count[_state];

in next iteration, _mb_id-- results in _mb_id == -1UL. -1UL > _vm->sbm.first_mb_id
is true, then the condition is still true. There, we have a invalid mb_id number.

>
> Otherwise we should always stay above _vm->sbm.first_mb_id.
>
> Am I correct or am I missing something?
>
>
> But in that case we would have to have
> 	vm->sbm.first_mb_id == 0
>
> and consequently the start address of the device would have to be at
> address 0.
>
> ... but that does sound very weird, especially on x86_64?
>
> Do you have details about the device layout / position in guest physical
> address space?
Hope I've explained it clearly above :-)
>
> virtio_mem_bbm_for_each_bb_rev() would need a similar fix.
Oh yes, thanks!

Eric
>
>
>> The calltrace is like below:
>> ```
>> [  286.344977] BUG: unable to handle page fault for address: ffffa95180cf8fff
>> [  286.345800] #PF: supervisor read access in kernel mode
>> [  286.346738] #PF: error_code(0x0000) - not-present page
>> [  286.347440] PGD 1000067 P4D 1000067 PUD 138c067 PMD 1840435067 PTE 0
>> [  286.348156] Oops: 0000 [#1] SMP PTI                                                        [  286.348556] CPU: 1 PID: 122 Comm: kworker/1:2 Tainted: G           OE
>> ...
>> [  286.350740] Workqueue: events_freezable virtio_mem_run_wq [virtio_mem]
>> [  286.351605] RIP: 0010:virtio_mem_unplug_request+0x418/0x890 [virtio_mem]
>> [  286.352519] Code: 0f 87 fc 00 00 00 4a 63 54 ac 30 48 83 bc d5 f8 00 00 00 00 48 89 d0 0f 8
>> 4 e5 00 00 00 48 8b b5 38 01 00 00 4c 89 e2 48 29 ca <0f> b6 34 16 39 c6 75 c7 40 80 fe 02 0f
>> 82 a4 01 00 00 40 80 fe 03
>> [  286.355030] RSP: 0018:ffffa95181c4bd50 EFLAGS: 00010286
>> [  286.355737] RAX: 0000000000000005 RBX: 0000000000006100 RCX: 0000000000000000
>> [  286.356752] RDX: ffffffffffffffff RSI: ffffa95180cf9000 RDI: ffff8e5dc393b348
>> [  286.357649] RBP: ffff8e5dc393b200 R08: ffff8e463cd2b610 R09: 0000000000000021
>> [  286.358627] R10: ffffa95181c4bcd0 R11: ffffa95181c4baa0 R12: ffffffffffffffff
>> [  286.359617] R13: 0000000000000003 R14: ffff8e5dc393b348 R15: 00000000fffffff0
>> [  286.360503] FS:  0000000000000000(0000) GS:ffff8e463cd00000(0000) knlGS:0000000000000000
>> [  286.361532] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
>> [  286.362229] CR2: ffffa95180cf8fff CR3: 0000001846234000 CR4: 00000000000006e0
>> [  286.363168] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
>> [  286.364162] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
>> [  286.365054] Call Trace:
>> [  286.365431]  ? virtio_mem_run_wq+0x5a4/0x870 [virtio_mem]
>> [  286.366132]  ? __schedule+0x4b3/0x800
>> [  286.366547]  ? process_one_work+0x18b/0x350
>> [  286.367041]  ? worker_thread+0x4f/0x3a0
>> [  286.367675]  ? rescuer_thread+0x350/0x350
>> [  286.368234]  ? kthread+0xfa/0x130
>> [  286.368605]  ? kthread_create_worker_on_cpu+0x70/0x70
>> [  286.369155]  ? ret_from_fork+0x1f/0x30
>> ```
>>
>> Fixes by also checking its up boundary.
>>
>> Signed-off-by: Eric Ren <renzhengeek@gmail.com>
>> --
>> v2: Correct the SOB
>>      Use _vm and remove duplicated spaces
>> ---
>>   drivers/virtio/virtio_mem.c | 4 +++-
>>   1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/virtio/virtio_mem.c b/drivers/virtio/virtio_mem.c
>> index 96e5a8782769..f1ba0dadd47a 100644
>> --- a/drivers/virtio/virtio_mem.c
>> +++ b/drivers/virtio/virtio_mem.c
>> @@ -486,7 +486,9 @@ static int virtio_mem_sbm_mb_states_prepare_next_mb(struct virtio_mem *vm)
>>   
>>   #define virtio_mem_sbm_for_each_mb_rev(_vm, _mb_id, _state) \
>>   	for (_mb_id = _vm->sbm.next_mb_id - 1; \
>> -	     _mb_id >= _vm->sbm.first_mb_id && _vm->sbm.mb_count[_state]; \
>> +	     _mb_id >= _vm->sbm.first_mb_id && \
>> +	     _mb_id < _vm->sbm.next_mb_id && \
>> +	     _vm->sbm.mb_count[_state]; \
>>   	     _mb_id--) \
>>   		if (virtio_mem_sbm_get_mb_state(_vm, _mb_id) == _state)
>>   
>


^ permalink raw reply	[flat|nested] 5+ messages in thread
* Re: [PATCH v2] virtio_mem: fix panic on mb_states indexing overflow
  2021-12-21 12:39   ` Eric Ren
@ 2021-12-21 13:53     ` David Hildenbrand
  2021-12-21 14:24       ` Eric Ren
  0 siblings, 1 reply; 5+ messages in thread
From: David Hildenbrand @ 2021-12-21 13:53 UTC (permalink / raw)
  To: Eric Ren, linux-mm; +Cc: linux-kernel, vbabka, ziy

Hi Eric,

> _vm->sbm.first_mb_id == 0

^ that's precisely what I meant.

How could it *ever* be safe on x86-64 to let a virtio-mem device start
on physical address 0, eventually overlapping essentially all DMA, the
BIOS and the PCI hole.

Thus my question: Is this a "fix" for x86-64 or is this a "prepare for"
for !x86-64 (e.g., arm64).

If it's a fix, we want proper "Fixes:" and "Cc: stable" tags. But I
assume this is much rather a preparation for another architecture than
x86-64.

Thanks!

-- 
Thanks,

David / dhildenb


^ permalink raw reply	[flat|nested] 5+ messages in thread
* Re: [PATCH v2] virtio_mem: fix panic on mb_states indexing overflow
  2021-12-21 13:53     ` David Hildenbrand
@ 2021-12-21 14:24       ` Eric Ren
  0 siblings, 0 replies; 5+ messages in thread
From: Eric Ren @ 2021-12-21 14:24 UTC (permalink / raw)
  To: David Hildenbrand, linux-mm; +Cc: linux-kernel, vbabka, ziy

Hi David,

在 12/21/21 9:53 PM, David Hildenbrand 写道:
> Hi Eric,
>
>> _vm->sbm.first_mb_id == 0
> ^ that's precisely what I meant.
Ah, finally got it!
>
> How could it *ever* be safe on x86-64 to let a virtio-mem device start
> on physical address 0, eventually overlapping essentially all DMA, the
> BIOS and the PCI hole.
You're right! That clears my doubt why this "issue" can live so long 
there... util I see it.

Yes, my setup is a very hacking enviroment. The device region start addr 
is wrongly
assigned to 0!

So, let's just drop this patch.

Eric
>
> Thus my question: Is this a "fix" for x86-64 or is this a "prepare for"
> for !x86-64 (e.g., arm64).
>
> If it's a fix, we want proper "Fixes:" and "Cc: stable" tags. But I
> assume this is much rather a preparation for another architecture than
> x86-64.
>
> Thanks!
>


^ permalink raw reply	[flat|nested] 5+ messages in thread
end of thread, other threads:[~2021-12-21 14:24 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-12-20 13:18 [PATCH v2] virtio_mem: fix panic on mb_states indexing overflow Eric Ren
2021-12-20 14:15 ` David Hildenbrand
2021-12-21 12:39   ` Eric Ren
2021-12-21 13:53     ` David Hildenbrand
2021-12-21 14:24       ` Eric Ren

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.