hw/cxl: Fix read from bogus memory

[PATCH] hw/cxl: Fix read from bogus memory

Posted by Ira Weiny 5 months, 3 weeks ago

Peter and coverity report:

	We've passed '&data' to address_space_write(), which means "read
	from the address on the stack where the function argument 'data'
	lives", so instead of writing 64 bytes of data to the guest ,
	we'll write 64 bytes which start with a host pointer value and
	then continue with whatever happens to be on the host stack
	after that.

Indeed the intention was to write 64 bytes of data at the address given.

Fix the parameter to address_space_write().

Reported-by: Peter Maydell <peter.maydell@linaro.org>
Link: https://lore.kernel.org/all/CAFEAcA-u4sytGwTKsb__Y+_+0O2-WwARntm3x8WNhvL1WfHOBg@mail.gmail.com/
Fixes: 6bda41a69bdc ("hw/cxl: Add clear poison mailbox command support.")
Cc: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Signed-off-by: Ira Weiny <ira.weiny@intel.com>
---
Compile tested only.  Jonathan please double check me.
---
 hw/mem/cxl_type3.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/mem/cxl_type3.c b/hw/mem/cxl_type3.c
index 3e42490b6ce8..582412d9925f 100644
--- a/hw/mem/cxl_type3.c
+++ b/hw/mem/cxl_type3.c
@@ -1025,7 +1025,7 @@ static bool set_cacheline(CXLType3Dev *ct3d, uint64_t dpa_offset, uint8_t *data)
         as = &ct3d->hostpmem_as;
     }
 
-    address_space_write(as, dpa_offset, MEMTXATTRS_UNSPECIFIED, &data,
+    address_space_write(as, dpa_offset, MEMTXATTRS_UNSPECIFIED, data,
                         CXL_CACHE_LINE_SIZE);
     return true;
 }

---
base-commit: 3b2fe44bb7f605f179e5e7feb2c13c2eb3abbb80
change-id: 20240531-fix-poison-set-cacheline-e32bc1e74b27

Best regards,
-- 
Ira Weiny <ira.weiny@intel.com>

Re: [PATCH] hw/cxl: Fix read from bogus memory

Posted by Philippe Mathieu-Daudé 5 months, 3 weeks ago

On 31/5/24 18:22, Ira Weiny wrote:
> Peter and coverity report:
> 
> 	We've passed '&data' to address_space_write(), which means "read
> 	from the address on the stack where the function argument 'data'
> 	lives", so instead of writing 64 bytes of data to the guest ,
> 	we'll write 64 bytes which start with a host pointer value and
> 	then continue with whatever happens to be on the host stack
> 	after that.
> 
> Indeed the intention was to write 64 bytes of data at the address given.
> 
> Fix the parameter to address_space_write().
> 
> Reported-by: Peter Maydell <peter.maydell@linaro.org>
> Link: https://lore.kernel.org/all/CAFEAcA-u4sytGwTKsb__Y+_+0O2-WwARntm3x8WNhvL1WfHOBg@mail.gmail.com/
> Fixes: 6bda41a69bdc ("hw/cxl: Add clear poison mailbox command support.")
> Cc: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Signed-off-by: Ira Weiny <ira.weiny@intel.com>
> ---
> Compile tested only.  Jonathan please double check me.
> ---
>   hw/mem/cxl_type3.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)

Thanks, patch queued.

Re: [PATCH] hw/cxl: Fix read from bogus memory

Posted by Michael S. Tsirkin 5 months, 3 weeks ago

On Mon, Jun 03, 2024 at 02:53:35PM +0200, Philippe Mathieu-Daudé wrote:
> On 31/5/24 18:22, Ira Weiny wrote:
> > Peter and coverity report:
> > 
> > 	We've passed '&data' to address_space_write(), which means "read
> > 	from the address on the stack where the function argument 'data'
> > 	lives", so instead of writing 64 bytes of data to the guest ,
> > 	we'll write 64 bytes which start with a host pointer value and
> > 	then continue with whatever happens to be on the host stack
> > 	after that.
> > 
> > Indeed the intention was to write 64 bytes of data at the address given.
> > 
> > Fix the parameter to address_space_write().
> > 
> > Reported-by: Peter Maydell <peter.maydell@linaro.org>
> > Link: https://lore.kernel.org/all/CAFEAcA-u4sytGwTKsb__Y+_+0O2-WwARntm3x8WNhvL1WfHOBg@mail.gmail.com/
> > Fixes: 6bda41a69bdc ("hw/cxl: Add clear poison mailbox command support.")
> > Cc: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> > Signed-off-by: Ira Weiny <ira.weiny@intel.com>
> > ---
> > Compile tested only.  Jonathan please double check me.
> > ---
> >   hw/mem/cxl_type3.c | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> Thanks, patch queued.

Had it queued too but sure, I can drop.

-- 
MST

Re: [PATCH] hw/cxl: Fix read from bogus memory

Posted by Philippe Mathieu-Daudé 5 months, 3 weeks ago

On 3/6/24 17:05, Michael S. Tsirkin wrote:
> On Mon, Jun 03, 2024 at 02:53:35PM +0200, Philippe Mathieu-Daudé wrote:
>> On 31/5/24 18:22, Ira Weiny wrote:
>>> Peter and coverity report:
>>>
>>> 	We've passed '&data' to address_space_write(), which means "read
>>> 	from the address on the stack where the function argument 'data'
>>> 	lives", so instead of writing 64 bytes of data to the guest ,
>>> 	we'll write 64 bytes which start with a host pointer value and
>>> 	then continue with whatever happens to be on the host stack
>>> 	after that.
>>>
>>> Indeed the intention was to write 64 bytes of data at the address given.
>>>
>>> Fix the parameter to address_space_write().
>>>
>>> Reported-by: Peter Maydell <peter.maydell@linaro.org>
>>> Link: https://lore.kernel.org/all/CAFEAcA-u4sytGwTKsb__Y+_+0O2-WwARntm3x8WNhvL1WfHOBg@mail.gmail.com/
>>> Fixes: 6bda41a69bdc ("hw/cxl: Add clear poison mailbox command support.")
>>> Cc: Jonathan Cameron <Jonathan.Cameron@huawei.com>
>>> Signed-off-by: Ira Weiny <ira.weiny@intel.com>
>>> ---
>>> Compile tested only.  Jonathan please double check me.
>>> ---
>>>    hw/mem/cxl_type3.c | 2 +-
>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> Thanks, patch queued.
> 
> Had it queued too but sure, I can drop.

Sorry I didn't notice that along with your R-b tag, my bad.

Don't modify your branch, I'm dropping it.

Regards,

Phil.

Re: [PATCH] hw/cxl: Fix read from bogus memory

Posted by Jonathan Cameron via 5 months, 3 weeks ago

On Fri, 31 May 2024 11:22:05 -0500
Ira Weiny <ira.weiny@intel.com> wrote:

> Peter and coverity report:
> 
> 	We've passed '&data' to address_space_write(), which means "read
> 	from the address on the stack where the function argument 'data'
> 	lives", so instead of writing 64 bytes of data to the guest ,
> 	we'll write 64 bytes which start with a host pointer value and
> 	then continue with whatever happens to be on the host stack
> 	after that.
> 
> Indeed the intention was to write 64 bytes of data at the address given.
> 
> Fix the parameter to address_space_write().
> 
> Reported-by: Peter Maydell <peter.maydell@linaro.org>
> Link: https://lore.kernel.org/all/CAFEAcA-u4sytGwTKsb__Y+_+0O2-WwARntm3x8WNhvL1WfHOBg@mail.gmail.com/
> Fixes: 6bda41a69bdc ("hw/cxl: Add clear poison mailbox command support.")
> Cc: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Signed-off-by: Ira Weiny <ira.weiny@intel.com>
> ---
> Compile tested only.  Jonathan please double check me.

Looks good to me.

Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

> ---
>  hw/mem/cxl_type3.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/mem/cxl_type3.c b/hw/mem/cxl_type3.c
> index 3e42490b6ce8..582412d9925f 100644
> --- a/hw/mem/cxl_type3.c
> +++ b/hw/mem/cxl_type3.c
> @@ -1025,7 +1025,7 @@ static bool set_cacheline(CXLType3Dev *ct3d, uint64_t dpa_offset, uint8_t *data)
>          as = &ct3d->hostpmem_as;
>      }
>  
> -    address_space_write(as, dpa_offset, MEMTXATTRS_UNSPECIFIED, &data,
> +    address_space_write(as, dpa_offset, MEMTXATTRS_UNSPECIFIED, data,
>                          CXL_CACHE_LINE_SIZE);
>      return true;
>  }
> 
> ---
> base-commit: 3b2fe44bb7f605f179e5e7feb2c13c2eb3abbb80
> change-id: 20240531-fix-poison-set-cacheline-e32bc1e74b27
> 
> Best regards,

Re: [PATCH] hw/cxl: Fix read from bogus memory

Posted by Michael S. Tsirkin 5 months, 3 weeks ago

On Fri, May 31, 2024 at 11:22:05AM -0500, Ira Weiny wrote:
> Peter and coverity report:
> 
> 	We've passed '&data' to address_space_write(), which means "read
> 	from the address on the stack where the function argument 'data'
> 	lives", so instead of writing 64 bytes of data to the guest ,
> 	we'll write 64 bytes which start with a host pointer value and
> 	then continue with whatever happens to be on the host stack
> 	after that.
> 
> Indeed the intention was to write 64 bytes of data at the address given.
> 
> Fix the parameter to address_space_write().
> 
> Reported-by: Peter Maydell <peter.maydell@linaro.org>
> Link: https://lore.kernel.org/all/CAFEAcA-u4sytGwTKsb__Y+_+0O2-WwARntm3x8WNhvL1WfHOBg@mail.gmail.com/
> Fixes: 6bda41a69bdc ("hw/cxl: Add clear poison mailbox command support.")
> Cc: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Signed-off-by: Ira Weiny <ira.weiny@intel.com>

Reviewed-by: Michael S. Tsirkin <mst@redhat.com>

I'll queue it for the next pull which should go out soonish.

> ---
> Compile tested only.  Jonathan please double check me.
> ---
>  hw/mem/cxl_type3.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/mem/cxl_type3.c b/hw/mem/cxl_type3.c
> index 3e42490b6ce8..582412d9925f 100644
> --- a/hw/mem/cxl_type3.c
> +++ b/hw/mem/cxl_type3.c
> @@ -1025,7 +1025,7 @@ static bool set_cacheline(CXLType3Dev *ct3d, uint64_t dpa_offset, uint8_t *data)
>          as = &ct3d->hostpmem_as;
>      }
>  
> -    address_space_write(as, dpa_offset, MEMTXATTRS_UNSPECIFIED, &data,
> +    address_space_write(as, dpa_offset, MEMTXATTRS_UNSPECIFIED, data,
>                          CXL_CACHE_LINE_SIZE);
>      return true;
>  }
> 
> ---
> base-commit: 3b2fe44bb7f605f179e5e7feb2c13c2eb3abbb80
> change-id: 20240531-fix-poison-set-cacheline-e32bc1e74b27
> 
> Best regards,
> -- 
> Ira Weiny <ira.weiny@intel.com>

Re: [PATCH] hw/cxl: Fix read from bogus memory

Posted by Peter Maydell 5 months, 3 weeks ago

On Fri, 31 May 2024 at 17:22, Ira Weiny <ira.weiny@intel.com> wrote:
>
> Peter and coverity report:
>
>         We've passed '&data' to address_space_write(), which means "read
>         from the address on the stack where the function argument 'data'
>         lives", so instead of writing 64 bytes of data to the guest ,
>         we'll write 64 bytes which start with a host pointer value and
>         then continue with whatever happens to be on the host stack
>         after that.
>
> Indeed the intention was to write 64 bytes of data at the address given.
>
> Fix the parameter to address_space_write().
>

Coverity CID: 1544772

> Reported-by: Peter Maydell <peter.maydell@linaro.org>
> Link: https://lore.kernel.org/all/CAFEAcA-u4sytGwTKsb__Y+_+0O2-WwARntm3x8WNhvL1WfHOBg@mail.gmail.com/
> Fixes: 6bda41a69bdc ("hw/cxl: Add clear poison mailbox command support.")
> Cc: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Signed-off-by: Ira Weiny <ira.weiny@intel.com>
> ---
> Compile tested only.  Jonathan please double check me.
> ---
>  hw/mem/cxl_type3.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/hw/mem/cxl_type3.c b/hw/mem/cxl_type3.c
> index 3e42490b6ce8..582412d9925f 100644
> --- a/hw/mem/cxl_type3.c
> +++ b/hw/mem/cxl_type3.c
> @@ -1025,7 +1025,7 @@ static bool set_cacheline(CXLType3Dev *ct3d, uint64_t dpa_offset, uint8_t *data)
>          as = &ct3d->hostpmem_as;
>      }
>
> -    address_space_write(as, dpa_offset, MEMTXATTRS_UNSPECIFIED, &data,
> +    address_space_write(as, dpa_offset, MEMTXATTRS_UNSPECIFIED, data,
>                          CXL_CACHE_LINE_SIZE);
>      return true;
>  }

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM