[v1] softmmu/physmem: Fix input parameters for flatview_access_allowed()

[PATCH] softmmu/physmem: Fix input parameters for flatview_access_allowed()

Posted by Zhenzhong Duan 3 years, 7 months ago

The comment of flatview_access_allowed() suggests to pass address
within that memory region, this isn't ture in some call sites.

This makes qemu log in flatview_access_allowed() confusing and
potential risk if the input parameter will be checked in the future.

Fixes: 3ab6fdc91b72 ("softmmu/physmem: Introduce MemTxAttrs::memory field and MEMTX_ACCESS_ERROR")
Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
---
 softmmu/physmem.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/softmmu/physmem.c b/softmmu/physmem.c
index fb16be57a6c6..214cb04c8fc3 100644
--- a/softmmu/physmem.c
+++ b/softmmu/physmem.c
@@ -2850,7 +2850,7 @@ static MemTxResult flatview_write(FlatView *fv, hwaddr addr, MemTxAttrs attrs,
 
     l = len;
     mr = flatview_translate(fv, addr, &addr1, &l, true, attrs);
-    if (!flatview_access_allowed(mr, attrs, addr, len)) {
+    if (!flatview_access_allowed(mr, attrs, addr1, l)) {
         return MEMTX_ACCESS_ERROR;
     }
     return flatview_write_continue(fv, addr, attrs, buf, len,
@@ -2917,7 +2917,7 @@ static MemTxResult flatview_read(FlatView *fv, hwaddr addr,
 
     l = len;
     mr = flatview_translate(fv, addr, &addr1, &l, false, attrs);
-    if (!flatview_access_allowed(mr, attrs, addr, len)) {
+    if (!flatview_access_allowed(mr, attrs, addr1, l)) {
         return MEMTX_ACCESS_ERROR;
     }
     return flatview_read_continue(fv, addr, attrs, buf, len,
-- 
2.25.1

Re: [PATCH] softmmu/physmem: Fix input parameters for flatview_access_allowed()

Posted by David Hildenbrand 3 years, 6 months ago

On 22.06.22 03:28, Zhenzhong Duan wrote:
> The comment of flatview_access_allowed() suggests to pass address
> within that memory region, this isn't ture in some call sites.

s/ture/true/

> 
> This makes qemu log in flatview_access_allowed() confusing and
> potential risk if the input parameter will be checked in the future.
> 
> Fixes: 3ab6fdc91b72 ("softmmu/physmem: Introduce MemTxAttrs::memory field and MEMTX_ACCESS_ERROR")

My opinion is to not use fixes tags unless there is really something
broken -- a user-visible effect of a BUG exists.

> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
> ---
>  softmmu/physmem.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/softmmu/physmem.c b/softmmu/physmem.c
> index fb16be57a6c6..214cb04c8fc3 100644
> --- a/softmmu/physmem.c
> +++ b/softmmu/physmem.c
> @@ -2850,7 +2850,7 @@ static MemTxResult flatview_write(FlatView *fv, hwaddr addr, MemTxAttrs attrs,
>  
>      l = len;
>      mr = flatview_translate(fv, addr, &addr1, &l, true, attrs);
> -    if (!flatview_access_allowed(mr, attrs, addr, len)) {
> +    if (!flatview_access_allowed(mr, attrs, addr1, l)) {
>          return MEMTX_ACCESS_ERROR;
>      }
>      return flatview_write_continue(fv, addr, attrs, buf, len,
> @@ -2917,7 +2917,7 @@ static MemTxResult flatview_read(FlatView *fv, hwaddr addr,
>  
>      l = len;
>      mr = flatview_translate(fv, addr, &addr1, &l, false, attrs);
> -    if (!flatview_access_allowed(mr, attrs, addr, len)) {
> +    if (!flatview_access_allowed(mr, attrs, addr1, l)) {
>          return MEMTX_ACCESS_ERROR;
>      }
>      return flatview_read_continue(fv, addr, attrs, buf, len,

Reviewed-by: David Hildenbrand <david@redhat.com>

-- 
Thanks,

David / dhildenb

RE: [PATCH] softmmu/physmem: Fix input parameters for flatview_access_allowed()

Posted by Duan, Zhenzhong 3 years, 6 months ago


>-----Original Message-----
>From: David Hildenbrand <david@redhat.com>
>Sent: Friday, July 22, 2022 2:49 PM
>To: Duan, Zhenzhong <zhenzhong.duan@intel.com>; qemu-
>devel@nongnu.org
>Cc: pbonzini@redhat.com; peterx@redhat.com; f4bug@amsat.org
>Subject: Re: [PATCH] softmmu/physmem: Fix input parameters for
>flatview_access_allowed()
>
>On 22.06.22 03:28, Zhenzhong Duan wrote:
>> The comment of flatview_access_allowed() suggests to pass address
>> within that memory region, this isn't ture in some call sites.
>
>s/ture/true/

Will fix.

>
>>
>> This makes qemu log in flatview_access_allowed() confusing and
>> potential risk if the input parameter will be checked in the future.
>>
>> Fixes: 3ab6fdc91b72 ("softmmu/physmem: Introduce MemTxAttrs::memory
>> field and MEMTX_ACCESS_ERROR")
>
>My opinion is to not use fixes tags unless there is really something broken --
>a user-visible effect of a BUG exists.

Thanks David, I will remove it in v2.

Zhenzhong

>
>> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
>> ---
>>  softmmu/physmem.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/softmmu/physmem.c b/softmmu/physmem.c index
>> fb16be57a6c6..214cb04c8fc3 100644
>> --- a/softmmu/physmem.c
>> +++ b/softmmu/physmem.c
>> @@ -2850,7 +2850,7 @@ static MemTxResult flatview_write(FlatView *fv,
>> hwaddr addr, MemTxAttrs attrs,
>>
>>      l = len;
>>      mr = flatview_translate(fv, addr, &addr1, &l, true, attrs);
>> -    if (!flatview_access_allowed(mr, attrs, addr, len)) {
>> +    if (!flatview_access_allowed(mr, attrs, addr1, l)) {
>>          return MEMTX_ACCESS_ERROR;
>>      }
>>      return flatview_write_continue(fv, addr, attrs, buf, len, @@
>> -2917,7 +2917,7 @@ static MemTxResult flatview_read(FlatView *fv,
>> hwaddr addr,
>>
>>      l = len;
>>      mr = flatview_translate(fv, addr, &addr1, &l, false, attrs);
>> -    if (!flatview_access_allowed(mr, attrs, addr, len)) {
>> +    if (!flatview_access_allowed(mr, attrs, addr1, l)) {
>>          return MEMTX_ACCESS_ERROR;
>>      }
>>      return flatview_read_continue(fv, addr, attrs, buf, len,
>
>Reviewed-by: David Hildenbrand <david@redhat.com>
>
>--
>Thanks,
>
>David / dhildenb

Re: [PATCH] softmmu/physmem: Fix input parameters for flatview_access_allowed()

Posted by Peter Xu 3 years, 7 months ago

On Wed, Jun 22, 2022 at 09:28:39AM +0800, Zhenzhong Duan wrote:
> The comment of flatview_access_allowed() suggests to pass address
> within that memory region, this isn't ture in some call sites.
> 
> This makes qemu log in flatview_access_allowed() confusing and
> potential risk if the input parameter will be checked in the future.
> 
> Fixes: 3ab6fdc91b72 ("softmmu/physmem: Introduce MemTxAttrs::memory field and MEMTX_ACCESS_ERROR")
> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>

Reviewed-by: Peter Xu <peterx@redhat.com>

Thanks,

-- 
Peter Xu

RE: [PATCH] softmmu/physmem: Fix input parameters for flatview_access_allowed()

Posted by Duan, Zhenzhong 3 years, 6 months ago


>-----Original Message-----
>From: Peter Xu <peterx@redhat.com>
>Sent: Thursday, June 23, 2022 5:03 AM
>To: Duan, Zhenzhong <zhenzhong.duan@intel.com>
>Cc: qemu-devel@nongnu.org; pbonzini@redhat.com; david@redhat.com;
>f4bug@amsat.org
>Subject: Re: [PATCH] softmmu/physmem: Fix input parameters for
>flatview_access_allowed()
>
>On Wed, Jun 22, 2022 at 09:28:39AM +0800, Zhenzhong Duan wrote:
>> The comment of flatview_access_allowed() suggests to pass address
>> within that memory region, this isn't ture in some call sites.
>>
>> This makes qemu log in flatview_access_allowed() confusing and
>> potential risk if the input parameter will be checked in the future.
>>
>> Fixes: 3ab6fdc91b72 ("softmmu/physmem: Introduce MemTxAttrs::memory
>> field and MEMTX_ACCESS_ERROR")
>> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
>
>Reviewed-by: Peter Xu <peterx@redhat.com>
Hi,

No progress for a long time, any further comments? Thanks!

Zhenzhong