Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

c18n: Set curthread->tcb to real tcb upon thread start #2110

Merged
merged 1 commit into from
Jun 11, 2024
Merged

Conversation

dpgao
Copy link
Contributor

@dpgao dpgao commented Jun 6, 2024

When a pthread is created, RTLD stores in its tcb field a fake tcb-like structure that contains a capability to the actual tcb and a capability to the thread's stack lookup table. This is done so that when the thread is entered, it does not need to create a stack lookup table, which involves a memory allocation that cannot be performed without a stack lookup table in the first place.

This commit fixes a mistake where the tcb field is not restored to the real tcb upon thread entry.

@jrtc27
Copy link
Member

jrtc27 commented Jun 6, 2024

If rtld is screwing with curthread, why doesn’t it fix it up itself?

@dpgao
Copy link
Contributor Author

dpgao commented Jun 6, 2024

If rtld is screwing with curthread, why doesn’t it fix it up itself?

It certainly can. But right now RTLD is not aware of the internal structure of struct pthread. I'm not sure if it's a good idea to teach it about that.

@jrtc27
Copy link
Member

jrtc27 commented Jun 6, 2024

Then how does it store the fake value there in the first place?

@dpgao
Copy link
Contributor Author

dpgao commented Jun 6, 2024

Then how does it store the fake value there in the first place?

In _thr_alloc the value returned by _tcb_ctor is stored in the tcb field. _tcb_ctor calls into RTLD to create the (fake) tcb.

Copy link
Collaborator

@bsdjhb bsdjhb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would suggest rewording the commit message a bit which I think would be less confusing (I think it led to Jess's question), maybe something like:

c18n: Set curthread->tcb to real tcb upon entry

When a pthread is created, the TCB is allocated in _tcb_ctor by calling _rtld_allocate_tls().  In the case of c18n, RTLD allocates both a real TCB
and a fake tcb-like structure that contains a pointer to the real TCB and
a pointer to the new thread's stack lookup table.  This is done so that when
the thread is entered, it does not need to create a stack lookup table, which
involves a memory allocation that cannot be performed without a stack
lookup table in the first place.

This commit fixes a mistake where the TCB field was still pointing to the fake
tcb-like structure instead of the real TCB after thread entry.

A few points: 1) A restore operation is usually paired with some sort of prior save, so using restore here is confusing since the field was never previously set to the real TCB. Using "Set" is clearer. 2) This message still doesn't really explain to me why the TCB has to be wrong initially. The comment in the commit seems to suggest that we are not allocating the real TCB during _tcb_ctor() (as my proposed log above suggests) but perhaps instead the real TCB is allocated later when the new thread starts running? In particular, I don't understand why you have to have the fake TCB to allocate the stack table. Why can't you just allocate the stack table and real TCB using _rtld_allocate_tls() and still return a pointer to the real TCB from that function?

@dpgao
Copy link
Contributor Author

dpgao commented Jun 6, 2024

In particular, I don't understand why you have to have the fake TCB to allocate the stack table. Why can't you just allocate the stack table and real TCB using _rtld_allocate_tls() and still return a pointer to the real TCB from that function?

The reason is that if I allocate the stack table in _rtld_allocate_tls() but just return a pointer to the real TCB, there would seem to be no way for me to retrieve the stack lookup table upon thread entry, so I have to embed a capability to the stack lookup table in the parameters passed to the kernel when I create a new thread.

@bsdjhb
Copy link
Collaborator

bsdjhb commented Jun 6, 2024

In particular, I don't understand why you have to have the fake TCB to allocate the stack table. Why can't you just allocate the stack table and real TCB using _rtld_allocate_tls() and still return a pointer to the real TCB from that function?

The reason is that if I allocate the stack table in _rtld_allocate_tls() but just return a pointer to the real TCB, there would seem to be no way for me to retrieve the stack lookup table upon thread entry, so I have to embed a capability to the stack lookup table in the parameters passed to the kernel when I create a new thread.

In particular, I don't understand why you have to have the fake TCB to allocate the stack table. Why can't you just allocate the stack table and real TCB using _rtld_allocate_tls() and still return a pointer to the real TCB from that function?

The reason is that if I allocate the stack table in _rtld_allocate_tls() but just return a pointer to the real TCB, there would seem to be no way for me to retrieve the stack lookup table upon thread entry, so I have to embed a capability to the stack lookup table in the parameters passed to the kernel when I create a new thread.

That would be a useful detail in the commit log then. :) Also, there should be a comment to that effect in c18n_allocate_tcb that cross references _rtld_thread_start, maybe something like:

/*
 * Create a fake wrapper tcb containing pointers to the real TCB
 * and stack table.  This is passed to the new thread as its initial
 * TCB.  Once the thread starts executing in _rtld_thread_start
 * it will set the thread's TCB to the real TCB after initializing its
 * stack table.
 */

@dpgao dpgao requested a review from bsdjhb June 7, 2024 09:03
@dpgao dpgao changed the title c18n: Restored curthread->tcb to real tcb upon thread entry c18n: Set curthread->tcb to real tcb upon thread start Jun 7, 2024
When a pthread is created, the TCB is allocated in _tcb_ctor by calling
_rtld_allocate_tls(). In the case of c18n, RTLD allocates a real TCB and
the new thread's stack lookup table and returns a fake wrapper TCB
containing both.

This is done so that when the thread is entered, it can readily install
the stack lookup table contained in the fake TCB instead of having to
allocate it---an action that cannot be performed without a stack lookup
table in the first place.

This commit fixes a mistake where the TCB field of curthread was still
pointing to the fake wrapper TCB structure instead of the real TCB after
thread start.
@dpgao dpgao merged commit 88cc22e into dev Jun 11, 2024
5 checks passed
@dpgao dpgao deleted the c18n-tcb-fix branch June 11, 2024 00:07
bsdjhb pushed a commit to bsdjhb/cheribsd that referenced this pull request Aug 20, 2024
Security fixes:
 CTSRD-CHERI#2135 rar: Fix OOB in rar e8 filter (CVE-2024-26256)
 CTSRD-CHERI#2145 zip: Fix out of boundary access

Important bugfixes:
 CTSRD-CHERI#2131 7zip: Limit amount of properties
 CTSRD-CHERI#2110 bsdtar: Fix error handling around strtol() usages
 CTSRD-CHERI#2116 passphrase: Never allow empty passwords
 CTSRD-CHERI#2124 rar: Fix "File CRC Error" when extracting specific rar4 archives
 CTSRD-CHERI#2123 xar: Avoid infinite link loop
 CTSRD-CHERI#2108 zip: Update AppleDouble support for directories
 CTSRD-CHERI#2071 zstd: Implement core detection

Obained from:		libarchive
Libarchive commit:	313aa1fa10b657de791e3202c168a6c833bc3543
bsdjhb pushed a commit to bsdjhb/cheribsd that referenced this pull request Aug 20, 2024
Libarchive 3.7.4 + three fixes from master

Security fixes:
 CTSRD-CHERI#2135 rar: Fix OOB in rar e8 filter (CVE-2024-26256)
 CTSRD-CHERI#2145 zip: Fix out of boundary access
 CTSRD-CHERI#2148 rar: Fix OOB in rar delta filter
 CTSRD-CHERI#2149 rar: Fix OOB in rar audio filter

Important bugfixes:
 CTSRD-CHERI#2131 7zip: Limit amount of properties
 CTSRD-CHERI#2110 bsdtar: Fix error handling around strtol() usages
 CTSRD-CHERI#2116 passphrase: Never allow empty passwords
 CTSRD-CHERI#2124 rar: Fix "File CRC Error" when extracting specific rar4 archives
 CTSRD-CHERI#2123 xar: Avoid infinite link loop
 CTSRD-CHERI#2150 xar: Fix another infinite loop and expat error handling
 CTSRD-CHERI#2108 zip: Update AppleDouble support for directories
 CTSRD-CHERI#2071 zstd: Implement core detectiongit

PR:		278588 (exp-run)
MFC after:	1 day
bsdjhb pushed a commit to bsdjhb/cheribsd that referenced this pull request Aug 20, 2024
Libarchive 3.7.4 + three fixes from master

Security fixes:
 CTSRD-CHERI#2135 rar: Fix OOB in rar e8 filter (CVE-2024-26256)
 CTSRD-CHERI#2145 zip: Fix out of boundary access
 CTSRD-CHERI#2148 rar: Fix OOB in rar delta filter
 CTSRD-CHERI#2149 rar: Fix OOB in rar audio filter

Important bugfixes:
 CTSRD-CHERI#2131 7zip: Limit amount of properties
 CTSRD-CHERI#2110 bsdtar: Fix error handling around strtol() usages
 CTSRD-CHERI#2116 passphrase: Never allow empty passwords
 CTSRD-CHERI#2124 rar: Fix "File CRC Error" when extracting specific rar4 archives
 CTSRD-CHERI#2123 xar: Avoid infinite link loop
 CTSRD-CHERI#2150 xar: Fix another infinite loop and expat error handling
 CTSRD-CHERI#2108 zip: Update AppleDouble support for directories
 CTSRD-CHERI#2071 zstd: Implement core detectiongit

PR:		278588 (exp-run)
MFC after:	1 day
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants