[PATCH] scsi: target: tcmu: Fix crash on ARM during cmd completion

[PATCH] scsi: target: tcmu: Fix crash on ARM during cmd completion

[Bodo Stroesser]

If tcmu_handle_completions() has to process a padding shorter
than sizeof(struct tcmu_cmd_entry), the current call to
tcmu_flush_dcache_range() with sizeof(struct tcmu_cmd_entry) as
length param is wrong and causes crashes on e.g. ARM, because
tcmu_flush_dcache_range() in this case calls
	flush_dcache_page(vmalloc_to_page(start));
with start being an invalid address above the end of the
vmalloc'ed area.

The fix is to use the maximum of remaining ring space and
sizeof(struct tcmu_cmd_entry) as the length param.

The patch was tested on kernel 4.19.118.

See https://bugzilla.kernel.org/show_bug.cgi?id=208045#c10

Signed-off-by: Bodo Stroesser <bstroesser@ts.fujitsu.com>
Tested-by: JiangYu <lnsyyj@hotmail.com>
---
 drivers/target/target_core_user.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/target/target_core_user.c b/drivers/target/target_core_user.c
index 3885ca532f8f..82e476d48194 100644
--- a/drivers/target/target_core_user.c
+++ b/drivers/target/target_core_user.c
@@ -1221,7 +1221,14 @@ static unsigned int tcmu_handle_completions(struct tcmu_dev *udev)
 
 		struct tcmu_cmd_entry *entry = (void *) mb + CMDR_OFF + udev->cmdr_last_cleaned;
 
-		tcmu_flush_dcache_range(entry, sizeof(*entry));
+		/*
+		 * Flush max. up to end of cmd ring, since current entry might
+		 * be a padding that is shorter than sizeof(*entry)
+		 */
+		size_t ring_left = head_to_end(udev->cmdr_last_cleaned,
+					       udev->cmdr_size);
+		tcmu_flush_dcache_range(entry, ring_left < sizeof(*entry) ?
+					ring_left : sizeof(*entry));
 
 		if (tcmu_hdr_get_op(entry->hdr.len_op) == TCMU_OP_PAD) {
 			UPDATE_HEAD(udev->cmdr_last_cleaned,
-- 
2.12.3

Re: [PATCH] scsi: target: tcmu: Fix crash on ARM during cmd completion

[Mike Christie]

On 6/24/20 3:53 AM, Bodo Stroesser wrote:
> If tcmu_handle_completions() has to process a padding shorter
> than sizeof(struct tcmu_cmd_entry), the current call to
> tcmu_flush_dcache_range() with sizeof(struct tcmu_cmd_entry) as
> length param is wrong and causes crashes on e.g. ARM, because
> tcmu_flush_dcache_range() in this case calls
> 	flush_dcache_page(vmalloc_to_page(start));
> with start being an invalid address above the end of the
> vmalloc'ed area.
> 
> The fix is to use the maximum of remaining ring space and
> sizeof(struct tcmu_cmd_entry) as the length param.
> 
> The patch was tested on kernel 4.19.118.
> 
> See https://urldefense.com/v3/__https://bugzilla.kernel.org/show_bug.cgi?id=208045*c10__;Iw!!GqivPVa7Brio!Kxr99oE0H1b9Ily4SE23nDN7ElSf8Tclo1RILfNSXb6iPh6DA5cSgtBQQsLMBKdrLsmT$ 
> 
> Signed-off-by: Bodo Stroesser <bstroesser@ts.fujitsu.com>
> Tested-by: JiangYu <lnsyyj@hotmail.com>
> ---
>  drivers/target/target_core_user.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/target/target_core_user.c b/drivers/target/target_core_user.c
> index 3885ca532f8f..82e476d48194 100644
> --- a/drivers/target/target_core_user.c
> +++ b/drivers/target/target_core_user.c
> @@ -1221,7 +1221,14 @@ static unsigned int tcmu_handle_completions(struct tcmu_dev *udev)
>  
>  		struct tcmu_cmd_entry *entry = (void *) mb + CMDR_OFF + udev->cmdr_last_cleaned;
>  
> -		tcmu_flush_dcache_range(entry, sizeof(*entry));
> +		/*
> +		 * Flush max. up to end of cmd ring, since current entry might
> +		 * be a padding that is shorter than sizeof(*entry)
> +		 */
> +		size_t ring_left = head_to_end(udev->cmdr_last_cleaned,
> +					       udev->cmdr_size);
> +		tcmu_flush_dcache_range(entry, ring_left < sizeof(*entry) ?
> +					ring_left : sizeof(*entry));
>  
>  		if (tcmu_hdr_get_op(entry->hdr.len_op) == TCMU_OP_PAD) {
>  			UPDATE_HEAD(udev->cmdr_last_cleaned,
> 

Thanks again.

Acked-by: Mike Christie <michael.christie@oracle.com>

Re: [PATCH] scsi: target: tcmu: Fix crash on ARM during cmd completion

[Bart Van Assche]

On 2020-06-24 01:53, Bodo Stroesser wrote:
> The fix is to use the maximum of remaining ring space and
> sizeof(struct tcmu_cmd_entry) as the length param.
> 

[ ... ]

> +		/*
> +		 * Flush max. up to end of cmd ring, since current entry might
> +		 * be a padding that is shorter than sizeof(*entry)
> +		 */
> +		size_t ring_left = head_to_end(udev->cmdr_last_cleaned,
> +					       udev->cmdr_size);
> +		tcmu_flush_dcache_range(entry, ring_left < sizeof(*entry) ?
> +					ring_left : sizeof(*entry));
>  
>  		if (tcmu_hdr_get_op(entry->hdr.len_op) == TCMU_OP_PAD) {
>  			UPDATE_HEAD(udev->cmdr_last_cleaned,

The patch description says "maximum" but the above formula calculates the
minimum of "ring_left" and sizeof(*entry). Did I perhaps misread this patch?

Thanks,

Bart.

Re: [PATCH] scsi: target: tcmu: Fix crash on ARM during cmd completion

[Michael Christie]

> On Jun 27, 2020, at 9:31 PM, Bart Van Assche <bvanassche@acm.org> wrote:
> 
> On 2020-06-24 01:53, Bodo Stroesser wrote:
>> The fix is to use the maximum of remaining ring space and
>> sizeof(struct tcmu_cmd_entry) as the length param.
>> 
> 
> [ ... ]
> 
>> +		/*
>> +		 * Flush max. up to end of cmd ring, since current entry might
>> +		 * be a padding that is shorter than sizeof(*entry)
>> +		 */
>> +		size_t ring_left = head_to_end(udev->cmdr_last_cleaned,
>> +					       udev->cmdr_size);
>> +		tcmu_flush_dcache_range(entry, ring_left < sizeof(*entry) ?
>> +					ring_left : sizeof(*entry));
>> 
>> 		if (tcmu_hdr_get_op(entry->hdr.len_op) == TCMU_OP_PAD) {
>> 			UPDATE_HEAD(udev->cmdr_last_cleaned,
> 
> The patch description says "maximum" but the above formula calculates the
> minimum of "ring_left" and sizeof(*entry). Did I perhaps misread this patch?

Ah yeah, Bodo probably meant to write what they wrote for the comment above about the max up to the end of the ring and not max of space left and entry size.

Re: [PATCH] scsi: target: tcmu: Fix crash on ARM during cmd completion

[Bodo Stroesser]

On 2020-06-28 21:35, Michael Christie wrote:
> 
> 
>> On Jun 27, 2020, at 9:31 PM, Bart Van Assche <bvanassche@acm.org> wrote:
>>
>> On 2020-06-24 01:53, Bodo Stroesser wrote:
>>> The fix is to use the maximum of remaining ring space and
>>> sizeof(struct tcmu_cmd_entry) as the length param.
>>>
>>
>> [ ... ]
>>
>>> +		/*
>>> +		 * Flush max. up to end of cmd ring, since current entry might
>>> +		 * be a padding that is shorter than sizeof(*entry)
>>> +		 */
>>> +		size_t ring_left = head_to_end(udev->cmdr_last_cleaned,
>>> +					       udev->cmdr_size);
>>> +		tcmu_flush_dcache_range(entry, ring_left < sizeof(*entry) ?
>>> +					ring_left : sizeof(*entry));
>>>
>>> 		if (tcmu_hdr_get_op(entry->hdr.len_op) == TCMU_OP_PAD) {
>>> 			UPDATE_HEAD(udev->cmdr_last_cleaned,
>>
>> The patch description says "maximum" but the above formula calculates the
>> minimum of "ring_left" and sizeof(*entry). Did I perhaps misread this patch?
> 
> Ah yeah, Bodo probably meant to write what they wrote for the comment above about the max up to the end of the ring and not max of space left and entry size.
> 

Thank you, you both are right.

While the code and the comment in the code are fine, patch description
is misleading or even wrong.

So I'm going to re-send the patch with fixed description and Mike's
Acked-by.

BR, Bodo