add GCC option for checking virtual table pointers

[Flo4152]

add GCC option for checking virtual table pointers

[Flo4152]

New pull request to add a section about virtual table checking section after synchronizing my repository with the base branch.

[Flo4152]

@thomasnyman @david-a-wheeler This pull request is a duplication of #440. I close my first pull request by mistake during the synchronization of my repository.

Sorry about this mistake.

Regards,

[thomasnyman]

@Flo4152 I've opened a pull request against your branch to address the linter errors as well as some consistency issues with the rest of the guide: https://github.com/Flo4152/wg-best-practices-os-developers/pull/1

Given the observation by @thesamesam in his https://github.com/ossf/wg-best-practices-os-developers/pull/485#discussion_r1594479772 regarding ABI breakage I think that is something that really warrants as When to not use? section I was asking for in the original PR

I am also wondering whether -fsanitize=vptr combined with the -fsanitize-minimal-runtime we were pointed to look at in https://github.com/ossf/wg-best-practices-os-developers/issues/326 would be a better options for production code.

That said, I do think the material on the usage of -fvtable-verify in this PR is good and given that there are examples of users of this, even though it may not be fit for a general recommendation, I do think this would be good material for the guide as long as the caveats with the option are clearly communicated.

[thesamesam]

My reservation is that I've genuinely never heard of anyone using this outside of those initial talks when it got merged into GCC. I'm sure users have existed, or do exist somewhere, but I've never heard from them and I don't ever see it mentioned in GCC bug reports or development discussions.

I'd feel a lot better about advertising this if:

• someone could ask around the GCC community (perhaps on the ML) what the consensus on the option is, and if it's suitable for the guide;
• someone could try build a bunch of software with it and see what happens (and make sure e.g. the compiler doesn't just ICE immediately)

I also don't think Google is using this anymore, given they're very much using the LLVM toolchain these days, but I could be wrong.

[david-a-wheeler]

These are really significant performance impacts, on a relatively narrowly focused countermeasure.

We definitely should document these options, including the performance impacts noted above. I don't think we should recommend using these options by default.

[Flo4152]

@david-a-wheeler I describe performance impacts in my last change of "Performance implications" subsection. I think I summarized number from SPECS benchmarks. Do you think there's anything missing or should be completed?

I agree theses options should not recommend using these options by default. Do you mean these options should be moved from the "Recommended compiler options that enable run-time protection mechanisms." table?

[david-a-wheeler]

Proposal - include text, but include at the end something like: “This option can have significant performance impact, while only countering a narrow type of vulnerability. Thus, we have not included this option as a recommended option in the current version of this guide.”

[thesamesam]

I'll stop after this and won't insist further, but I have to ask:

We definitely should document these options, including the performance impacts noted above. I don't think we should recommend using these options by default.

Why is this a definitely? Including things which we're saying people shouldn't use by default (and with a huge amount of caveats) is noise and it may end up reducing the effectiveness of the guide if it's a huge document with lots of irrelevant parts.

There are other options which have significant performance impact which are worth mentioning because they're actively developed and tested at least. But to me, this particular one appears all-negative (https://github.com/ossf/wg-best-practices-os-developers/pull/485#issuecomment-2118983435). There are various options which are also not-great ideas which I could make PRs for. What makes this one different?

Isn't part of the goal here to focus on modern, useful, and sensible options?

Not trying to be antagonistic here, I just really don't think this option constitutes "best practices" and I think mentioning it may end up drowning out important other recommendations.

(Note that the PR as-is adds it under "Table 2: Recommended compiler options that enable run-time protection mechanisms.", not "Discouraged Compiler Options").

EDIT: I took some informal soundings from GCC developers and the rough consensus was "we think it's unmaintained".

[Flo4152]

@thomasnyman @david-a-wheeler There are arguments against adding this flag to your guide. This pull request was discussed at your zoom calls, and adding this flag seems to be in line with what was said at those calls.

Do you still feel the same way about adding this flag?

[siddhesh]

It appears that the current state is the following:

• The -fvtable-verify option was implemented long ago with the idea that it may be useful, but never got any use in the Open Source ecosystem. Further, it appears that the feature is largely unmaintained since the feature implementors have since stopped contributing to GCC.
• The option in isolation breaks ABI, so it's unlikely that an individual project would be able to use it safely in any capacity.
• The option has a significant performance overhead

All of that sounds to me like this shouldn't go into the main guide, but should get mentioned in the Appendix as an option that was considered but rejected.

[thomasnyman]

@Flo4152: I would say that in general the attitude toward including the -fvtable-verify as a informational, but not as a recommendation in the guide has seen support in the Compiler BP calls but after reviewing this thread and the notes from the past meeting, it seems to me that everyone involved has reservations about recommending the -fvtable-verify option, which brings me around to think that @siddhesh proposal would be an acceptable conclusion from my perspective as well.

I'd hate to see the effort gone into putting together the detailed description go to waste and even though I think it would be acceptable to include options that would only be relevant for specialized ecosystems, e.g., in embedded or HCP environments, what makes me most uncomfortable with @siddhesh's characterization is the 'unmaintained' part. Ultimately our objective with the guide is to direct people towards the security-relevant options that are worth pursuing, and if the stance of the GCC community representatives is that the -fvtable-verify is not a relevant option any longer that has a lot of weight for me when forming my opinion.

On a positive note, I think this PR has raised a lot of good discussion around the background and status of this feature, and I applaud the due diligence that has gone into the research of this.

[thomasnyman]

In the interest of trying to wrap this topic up after a lengthy discussion, I've proposed some text to add to the list of considred options for -fvtable-verifyin #553.

[Flo4152]

@thomasnyman @siddhesh I agree. I'm closing this request.

[Flo4152]

Thank you all for your responses to this pull request. I appreciate the corrections, advice and help I received.

Regards,