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

TCTI: Separate namespaces for internal struct #2843

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

AndreasFuchsTPM
Copy link
Member

Some (dynamic) linkers seam to resolve internal symbols against the loading library in a dlopen() instead of its local symbols. Namely a tss2_tcti_info symbol ref of e.g. tcti-device would resolve against the symbol of tctildr.
We protect against such situations by separating the names.

Fixes: #2840

@AndreasFuchsTPM AndreasFuchsTPM added the backport Issues to be backported to old-stable label May 17, 2024
@AndreasFuchsTPM AndreasFuchsTPM added this to the 4.2.0 milestone May 17, 2024
joholl
joholl previously approved these changes May 17, 2024
@@ -622,7 +622,7 @@ TSS2_RC Tss2_Tcti_Cmd_Init (TSS2_TCTI_CONTEXT *tctiContext, size_t *size,
}

/* public info structure */
const TSS2_TCTI_INFO tss2_tcti_info = {
const TSS2_TCTI_INFO tss2_tcti_cmd_info = {
Copy link
Contributor

Choose a reason for hiding this comment

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

@AndreasFuchsTPM any reason not to make these static too, just to reduce the incidence of these?

@@ -846,7 +846,7 @@ TSS2_RC Tss2_Tcti_I2c_Helper_Init (TSS2_TCTI_CONTEXT* tcti_context, size_t* size
return TSS2_RC_SUCCESS;
}

static const TSS2_TCTI_INFO tss2_tcti_info = {
static const TSS2_TCTI_INFO tss2_tcti_i2c_helper_info = {
Copy link
Contributor

Choose a reason for hiding this comment

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

@AndreasFuchsTPM why is this one static, but the other ones aren't?

@AndreasFuchsTPM
Copy link
Member Author

AFAIR, the static qualifier does not change anything for global variables.
My understanding is:
static functions -> only visible inside local file
static (function) variables -> become global variables but under the namespace of the function
static (global) variables -> are global variables anyways, so no change given static

See also
image

If I move the info into the getInfo function:
image
image

That's at least, what GCC does.

My take away was always: the static keyword is misleading for global variables and thus should be omitted.

So yes, I'll get rid of these...

Some (dynamic) linkers seam to resolve internal symbols against
the loading library in a dlopen() instead of its local symbols.
Namely a tss2_tcti_info symbol ref of e.g. tcti-device would resolve
against the symbol of tctildr.
We protect against such situations by separating the names.

Signed-off-by: Andreas Fuchs <[email protected]>
@SoapGentoo
Copy link
Contributor

SoapGentoo commented May 23, 2024

diff -u <(echo 'const int x = 0;' | gcc -c -xc - -o non-static.o && readelf -a non-static.o) <(echo 'static const int x = 0;' | gcc -c -xc - -o static.o && readelf -a static.o)
--- /dev/fd/63
+++ /dev/fd/62
@@ -39,7 +39,7 @@
   [ 7] .note.gnu.pr[...] NOTE             0000000000000000  00000078
        0000000000000050  0000000000000000   A       0     0     8
   [ 8] .symtab           SYMTAB           0000000000000000  000000c8
-       0000000000000048  0000000000000018           9     2     8
+       0000000000000048  0000000000000018           9     3     8
   [ 9] .strtab           STRTAB           0000000000000000  00000110
        000000000000000b  0000000000000000           0     0     1
   [10] .shstrtab         STRTAB           0000000000000000  0000011b
@@ -63,7 +63,7 @@
    Num:    Value          Size Type    Bind   Vis      Ndx Name
      0: 0000000000000000     0 NOTYPE  LOCAL  DEFAULT  UND 
      1: 0000000000000000     0 FILE    LOCAL  DEFAULT  ABS <stdin>
-     2: 0000000000000000     4 OBJECT  GLOBAL DEFAULT    4 x
+     2: 0000000000000000     4 OBJECT  LOCAL  DEFAULT    4 x
 
 No version information found in this file.

static on global variables makes the linkage internal, which means less leaking of internal symbols, which in general is a good thing. I doubt this bug would have occurred if all instances of TSS2_TCTI_INFO objects would have been static in their respective files.

Final piece of why you should always use static when possible: https://godbolt.org/z/v1z14Ka1W
static global variables do not go through the GOT when compiled with -fPIC, this avoids a pointless indirection if you plan to keep the variable private anyhow.

@@ -622,7 +622,7 @@ TSS2_RC Tss2_Tcti_Cmd_Init (TSS2_TCTI_CONTEXT *tctiContext, size_t *size,
}

/* public info structure */
const TSS2_TCTI_INFO tss2_tcti_info = {
const TSS2_TCTI_INFO tss2_tcti_cmd_info = {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
const TSS2_TCTI_INFO tss2_tcti_cmd_info = {
static const TSS2_TCTI_INFO tss2_tcti_cmd_info = {

@@ -530,7 +530,7 @@ Tss2_Tcti_Device_Init (
return TSS2_RC_SUCCESS;
}

const TSS2_TCTI_INFO tss2_tcti_info = {
const TSS2_TCTI_INFO tss2_tcti_device_info = {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
const TSS2_TCTI_INFO tss2_tcti_device_info = {
static const TSS2_TCTI_INFO tss2_tcti_device_info = {

@@ -280,7 +280,7 @@ Tss2_Tcti_I2c_Ftdi_Init (TSS2_TCTI_CONTEXT* tcti_context, size_t* size, const ch
return Tss2_Tcti_I2c_Helper_Init (tcti_context, size, &tcti_platform);
}

const TSS2_TCTI_INFO tss2_tcti_info = {
const TSS2_TCTI_INFO tss2_tcti_i2c_ftdi_info = {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
const TSS2_TCTI_INFO tss2_tcti_i2c_ftdi_info = {
static const TSS2_TCTI_INFO tss2_tcti_i2c_ftdi_info = {

@@ -846,7 +846,7 @@ TSS2_RC Tss2_Tcti_I2c_Helper_Init (TSS2_TCTI_CONTEXT* tcti_context, size_t* size
return TSS2_RC_SUCCESS;
}

static const TSS2_TCTI_INFO tss2_tcti_info = {
const TSS2_TCTI_INFO tss2_tcti_i2c_helper_info = {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
const TSS2_TCTI_INFO tss2_tcti_i2c_helper_info = {
static const TSS2_TCTI_INFO tss2_tcti_i2c_helper_info = {

@@ -880,7 +880,7 @@ Tss2_Tcti_Libtpms_Init(
}

/* public info structure */
const TSS2_TCTI_INFO tss2_tcti_info = {
const TSS2_TCTI_INFO tss2_tcti_libtpms_info = {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
const TSS2_TCTI_INFO tss2_tcti_libtpms_info = {
static const TSS2_TCTI_INFO tss2_tcti_libtpms_info = {

@@ -197,7 +197,7 @@ Tss2_Tcti_Spidev_Init (TSS2_TCTI_CONTEXT* tcti_context, size_t* size, const char
return Tss2_Tcti_Spi_Helper_Init (tcti_context, size, &platform);
}

const TSS2_TCTI_INFO tss2_tcti_info = {
const TSS2_TCTI_INFO tss2_tcti_spidev_info = {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
const TSS2_TCTI_INFO tss2_tcti_spidev_info = {
static const TSS2_TCTI_INFO tss2_tcti_spidev_info = {

@@ -639,7 +639,7 @@ Tss2_Tcti_Swtpm_Init (
}

/* public info structure */
const TSS2_TCTI_INFO tss2_tcti_info = {
const TSS2_TCTI_INFO tss2_tcti_swtpm_info = {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
const TSS2_TCTI_INFO tss2_tcti_swtpm_info = {
static const TSS2_TCTI_INFO tss2_tcti_swtpm_info = {

@@ -315,7 +315,7 @@ Tss2_Tcti_Tbs_Init (
return TSS2_RC_SUCCESS;
}

const TSS2_TCTI_INFO tss2_tcti_info = {
const TSS2_TCTI_INFO tss2_tcti_tbs_info = {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
const TSS2_TCTI_INFO tss2_tcti_tbs_info = {
static const TSS2_TCTI_INFO tss2_tcti_tbs_info = {

@@ -587,7 +587,7 @@ TSS2_RC Tss2_Tcti_TctiLdr_Init (TSS2_TCTI_CONTEXT *tctiContext, size_t *size,
}

__attribute__((weak))
const TSS2_TCTI_INFO tss2_tcti_info = {
const TSS2_TCTI_INFO tss2_tctildr_info = {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
const TSS2_TCTI_INFO tss2_tctildr_info = {
static const TSS2_TCTI_INFO tss2_tctildr_info = {

@@ -265,7 +265,7 @@ Tss2_Tcti_Fuzzing_Init (
}

/* public info structure */
const TSS2_TCTI_INFO tss2_tcti_info = {
const TSS2_TCTI_INFO tss2_tcti_fuzzing_info = {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
const TSS2_TCTI_INFO tss2_tcti_fuzzing_info = {
static const TSS2_TCTI_INFO tss2_tcti_fuzzing_info = {

@SoapGentoo
Copy link
Contributor

@AndreasFuchsTPM ping

@SoapGentoo
Copy link
Contributor

@AndreasFuchsTPM any movement here?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport Issues to be backported to old-stable
Projects
None yet
Development

Successfully merging this pull request may close these issues.

>=4.1.0 Cannot load device TCTI when compiled with clang
3 participants