| Message ID | 20171123124718.16839-1-vaibhav@linux.vnet.ibm.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [1/2] powerpc: Avoid signed to unsigned conversion in set_thread_tidr() | expand |
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
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> 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
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.
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 */
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(-)