[PATCH 1/2] powerpc: Avoid signed to unsigned conversion in set_thread_tidr()

[PATCH 1/2] powerpc: Avoid signed to unsigned conversion in set_thread_tidr()

[Vaibhav Jain]

There is an unsafe signed to unsigned conversion in set_thread_tidr()
that may cause an error value to be assigned to SPRN_TIDR register and
used as thread-id.

The issue happens as assign_thread_tidr() returns an int and
thread.tidr is an unsigned-long. So a negative error code returned
from assign_thread_tidr() will fail the error check and gets assigned
as tidr as a large positive value.

To fix this the patch assigns the return value of assign_thread_tidr()
to a temporary int and assigns it to thread.tidr iff its '> 0'.

Fixes: ec233ede4c86("powerpc: Add support for setting SPRN_TIDR")
Signed-off-by: Vaibhav Jain <vaibhav@linux.vnet.ibm.com>
---
 arch/powerpc/kernel/process.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c
index bfdd783e3916..a6eaf924c8b6 100644
--- a/arch/powerpc/kernel/process.c
+++ b/arch/powerpc/kernel/process.c
@@ -1569,19 +1569,21 @@ void arch_release_task_struct(struct task_struct *t)
  */
 int set_thread_tidr(struct task_struct *t)
 {
+	int rc;
+
 	if (!cpu_has_feature(CPU_FTR_ARCH_300))
 		return -EINVAL;
 
 	if (t != current)
 		return -EINVAL;
 
-	t->thread.tidr = assign_thread_tidr();
-	if (t->thread.tidr < 0)
-		return t->thread.tidr;
-
-	mtspr(SPRN_TIDR, t->thread.tidr);
+	rc = assign_thread_tidr();
+	if (rc > 0) {
+		t->thread.tidr = assign_thread_tidr();
+		mtspr(SPRN_TIDR, t->thread.tidr);
+	}
 
-	return 0;
+	return rc;
 }
 
 #endif /* CONFIG_PPC64 */
-- 
2.14.3

[PATCH 2/2] powerpc: Do not assign thread.tidr if already assigned

[Vaibhav Jain]

If set_thread_tidr() is called twice for same task_struct then it will
allocated a new tidr value to it leaving the previous value still
dangling in the vas_thread_ida table.

To fix this the patch changes set_thread_tidr() checks if a tidr value
is already assigned to the task_struct and if yes then returns the
existing value from function instead of allocating a new one.

Fixes: ec233ede4c86("powerpc: Add support for setting SPRN_TIDR")
Signed-off-by: Vaibhav Jain <vaibhav@linux.vnet.ibm.com>
---
 arch/powerpc/kernel/process.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c
index a6eaf924c8b6..900193e4d6d8 100644
--- a/arch/powerpc/kernel/process.c
+++ b/arch/powerpc/kernel/process.c
@@ -1577,6 +1577,9 @@ int set_thread_tidr(struct task_struct *t)
 	if (t != current)
 		return -EINVAL;
 
+	if (t->thread.tidr)
+		return t->thread.tidr;
+
 	rc = assign_thread_tidr();
 	if (rc > 0) {
 		t->thread.tidr = assign_thread_tidr();
-- 
2.14.3

Re: [PATCH 1/2] powerpc: Avoid signed to unsigned conversion in set_thread_tidr()

[Michael Ellerman]

Vaibhav Jain <vaibhav@linux.vnet.ibm.com> writes:

> There is an unsafe signed to unsigned conversion in set_thread_tidr()
> that may cause an error value to be assigned to SPRN_TIDR register and
> used as thread-id.
>
> The issue happens as assign_thread_tidr() returns an int and
> thread.tidr is an unsigned-long. So a negative error code returned
> from assign_thread_tidr() will fail the error check and gets assigned
> as tidr as a large positive value.
>
> To fix this the patch assigns the return value of assign_thread_tidr()
> to a temporary int and assigns it to thread.tidr iff its '> 0'.

.. and changes the calling convention of the function.

Now it returns -ve error values, or a +ve TIDR value when it succeeds,
or possibly 0 if that's returned by assign_thread_tidr().

Which I'm not sure you meant to do. If you did, you should at least
document it.

But frankly I'd rather we left it the way it was, -ve error or 0 for
success.

cheers

> diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c
> index bfdd783e3916..a6eaf924c8b6 100644
> --- a/arch/powerpc/kernel/process.c
> +++ b/arch/powerpc/kernel/process.c
> @@ -1569,19 +1569,21 @@ void arch_release_task_struct(struct task_struct *t)
>   */
>  int set_thread_tidr(struct task_struct *t)
>  {
> +	int rc;
> +
>  	if (!cpu_has_feature(CPU_FTR_ARCH_300))
>  		return -EINVAL;
>  
>  	if (t != current)
>  		return -EINVAL;
>  
> -	t->thread.tidr = assign_thread_tidr();
> -	if (t->thread.tidr < 0)
> -		return t->thread.tidr;
> -
> -	mtspr(SPRN_TIDR, t->thread.tidr);
> +	rc = assign_thread_tidr();
> +	if (rc > 0) {
> +		t->thread.tidr = assign_thread_tidr();
> +		mtspr(SPRN_TIDR, t->thread.tidr);
> +	}
>  
> -	return 0;
> +	return rc;
>  }
>  
>  #endif /* CONFIG_PPC64 */
> -- 
> 2.14.3

Re: [PATCH 1/2] powerpc: Avoid signed to unsigned conversion in set_thread_tidr()

[Vaibhav Jain]

Thanks Mpe for reviewing the patch

Michael Ellerman <mpe@ellerman.id.au> writes:

>> To fix this the patch assigns the return value of assign_thread_tidr()
>> to a temporary int and assigns it to thread.tidr iff its '> 0'.
>
> .. and changes the calling convention of the function.
>
> Now it returns -ve error values, or a +ve TIDR value when it succeeds,
> or possibly 0 if that's returned by assign_thread_tidr().
>
> Which I'm not sure you meant to do. If you did, you should at least
> document it.
>

Yes this is intentional and this was supposed to be the calling
convention of set_thread_tidr() in first place. At-least that what I
gather from subsequent cxl patch to add its support
http://patchwork.ozlabs.org/patch/840719/

set_thread_tidr() is a wrapper around assign_thread_tidr() which follows
similar calling convention i.e return -ve values for error and  +ve
value successfully allocated tidr. The way assign_thread_tidr() is
implemented it will never return a '0'.

Currently set_thread_tidr() will wrongly assign an incorrect tidr value
to SPRN_TIDR in case assign_thread_tidr() returns an error. This patch
fixes this issue and should not impact the existing calling convention
of set_thread_tidr(). The patch should not have an impact on the calling
convention of set_thread_tidr().

I also have unintentionally left a tidr leak in this patch and will send
a v2 with the fix and also updating the patch description mentioning the
calling convention of the function.

-- 
Vaibhav Jain <vaibhav@linux.vnet.ibm.com>
Linux Technology Center, IBM India Pvt. Ltd.

Re: [PATCH 1/2] powerpc: Avoid signed to unsigned conversion in set_thread_tidr()

[Michael Ellerman]

Vaibhav Jain <vaibhav@linux.vnet.ibm.com> writes:

> Thanks Mpe for reviewing the patch
>
> Michael Ellerman <mpe@ellerman.id.au> writes:
>
>>> To fix this the patch assigns the return value of assign_thread_tidr()
>>> to a temporary int and assigns it to thread.tidr iff its '> 0'.
>>
>> .. and changes the calling convention of the function.
>>
>> Now it returns -ve error values, or a +ve TIDR value when it succeeds,
>> or possibly 0 if that's returned by assign_thread_tidr().
>>
>> Which I'm not sure you meant to do. If you did, you should at least
>> document it.
>
> Yes this is intentional and this was supposed to be the calling
> convention of set_thread_tidr() in first place. At-least that what I
> gather from subsequent cxl patch to add its support
> http://patchwork.ozlabs.org/patch/840719/

That's not at all what I gather from that patch.

+		/* Assign a unique TIDR (thread id) for the current thread */
+		rc = set_thread_tidr(current);
+		if (!rc)
+			ctx->tid = current->thread.tidr;

That expects 0 on success, anything else is an error.

Which is what set_thread_tidr() currently implements, and is the most
common calling convention in kernel code.

Please don't change that as part of an unrelated fix.

If you want to change the calling convention, send a patch to do that
and only that.

cheers

Re: [PATCH 1/2] powerpc: Avoid signed to unsigned conversion in set_thread_tidr()

[Vaibhav Jain]

Michael Ellerman <mpe@ellerman.id.au> writes:

> Vaibhav Jain <vaibhav@linux.vnet.ibm.com> writes:
>
>> Thanks Mpe for reviewing the patch
>>
>> Michael Ellerman <mpe@ellerman.id.au> writes:
>>
>>>> To fix this the patch assigns the return value of assign_thread_tidr()
>>>> to a temporary int and assigns it to thread.tidr iff its '> 0'.
>>>
>>> .. and changes the calling convention of the function.
>>>
>>> Now it returns -ve error values, or a +ve TIDR value when it succeeds,
>>> or possibly 0 if that's returned by assign_thread_tidr().
>>>
>>> Which I'm not sure you meant to do. If you did, you should at least
>>> document it.
>>
>> Yes this is intentional and this was supposed to be the calling
>> convention of set_thread_tidr() in first place. At-least that what I
>> gather from subsequent cxl patch to add its support
>> http://patchwork.ozlabs.org/patch/840719/
>
> That's not at all what I gather from that patch.
>
> +		/* Assign a unique TIDR (thread id) for the current thread */
> +		rc = set_thread_tidr(current);
> +		if (!rc)
> +			ctx->tid = current->thread.tidr;
>
> That expects 0 on success, anything else is an error.
>
> Which is what set_thread_tidr() currently implements, and is the most
> common calling convention in kernel code.
>
> Please don't change that as part of an unrelated fix.
>
> If you want to change the calling convention, send a patch to do that
> and only that.
>
> cheers
>
Agreed Mpe, checked with Christophe and he too echoed similar inputs. I
will update my v2 patch by not causing a change to he call convention.

-- 
Vaibhav Jain <vaibhav@linux.vnet.ibm.com>
Linux Technology Center, IBM India Pvt. Ltd.