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

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 <redacted>
---
 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

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 <redacted>
---
 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

Vaibhav Jain [off-list ref] 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

Thanks Mpe for reviewing the patch

Michael Ellerman [off-list ref] writes:

quoted

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 [off-list ref]
Linux Technology Center, IBM India Pvt. Ltd.

Vaibhav Jain [off-list ref] writes:

Thanks Mpe for reviewing the patch

Michael Ellerman [off-list ref] writes:

quoted

quoted

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

Michael Ellerman [off-list ref] writes:

Vaibhav Jain [off-list ref] writes:

quoted

Thanks Mpe for reviewing the patch

Michael Ellerman [off-list ref] writes:

quoted

quoted

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 [off-list ref]
Linux Technology Center, IBM India Pvt. Ltd.