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

Hardened C #1

Merged
merged 1 commit into from
Apr 24, 2024
Merged

Hardened C #1

merged 1 commit into from
Apr 24, 2024

Conversation

ericcurtin
Copy link
Contributor

It's not Rust, but. We can maximize our compiler warnings I guess.

@ericcurtin
Copy link
Contributor Author

I'm surprised this function builds:

$ grep "connect_to_passt" krun.c
int connect_to_passt()
        int passt_fd = cmdline.passt_socket_path ? connect_to_passt(cmdline.passt_socket_path) : start_passt();

signature seems different in caller and implementation.

@ericcurtin
Copy link
Contributor Author

Ah it's because when you use () without void, the compiler doesn't do strict signature checking. We could catch this with compiler warnings.

Anyway details, I understand you are just getting started on something exciting :)

@ericcurtin ericcurtin force-pushed the hardened_c branch 2 times, most recently from 194b804 to 6041952 Compare April 21, 2024 10:53
@ericcurtin
Copy link
Contributor Author

ericcurtin commented Apr 21, 2024

I've been thinking of trying out something like this on a new project:

#include <stdio.h>

#define UNSAFE_C_START _Pragma("GCC diagnostic push") \
                       _Pragma("GCC diagnostic ignored \"-Wunused-but-set-variable\"")

#define UNSAFE_C_END _Pragma("GCC diagnostic pop")

int main(void) {
UNSAFE_C_START
    int unused_variable;
UNSAFE_C_END

    // Code outside the suppressed block
    unused_variable = 5; // Warning: unused variable 'unusedVariable'

    return 0;
}

with close to maximum gcc/clang warnings turned on. This could be a fun project to try it on

It's not Rust, but. We can maximize our compiler warnings I guess.

Signed-off-by: Eric Curtin <[email protected]>
@teohhanhui
Copy link

It's not Rust, but.

Would there be any interest to rewrite this project in Rust while it's still very young? I'd surely like to contribute to that.

@slp
Copy link
Owner

slp commented Apr 24, 2024

@ericcurtin As you correctly guessed, I put this code together by borrowing from chroot_vm.c and init/init.c in a couple hours as a PoC. I wasn't intending to announce it in its current form, but since we're already there, let's get it in shape. Thanks for the PR!

@slp
Copy link
Owner

slp commented Apr 24, 2024

It's not Rust, but.

Would there be any interest to rewrite this project in Rust while it's still very young? I'd surely like to contribute to that.

Most of the code I write these days is in Rust, so this is tempting. But, with a cold head, most of what krun-guest.c is going to be doing is calling (more or less) bare syscalls, so the benefit of rewriting it in Rust is very small. For krun.c it'd be easier to justify, but in the end is still small wrapper on top of libkrun, which is already written in Rust.

The threat model for this project is also relatively low risk. It's never intended to be run as root, it doesn't gain privileges in any way, and doesn't operate as a service either.

All in all, I think we should keep it as simple as possible, at least for the time being.

@slp slp merged commit 26237c5 into slp:main Apr 24, 2024
@ericcurtin ericcurtin deleted the hardened_c branch April 24, 2024 16:09
@teohhanhui
Copy link

most of what krun-guest.c is going to be doing is calling (more or less) bare syscalls, so the benefit of rewriting it in Rust is very small.

I think for a greenfield project, there is value in writing it in Rust from the beginning, especially with Rust's move to I/O safety. There's also the point of just avoiding creating more C code that will need to be maintained into the future. Not to mention that libkrun is already written in Rust, so there's continuity in that sense that would encourage contribution.

All in all, I think we should keep it as simple as possible, at least for the time being.

It is simple, until it is not anymore.

I'd like to volunteer to do the work.

@slp
Copy link
Owner

slp commented Apr 25, 2024

All in all, I think we should keep it as simple as possible, at least for the time being.

It is simple, until it is not anymore.

I'd like to volunteer to do the work.

SGTM. I would be more than happy to delegate this work to someone else so I can focus on libkrun :-)

Once you have an initial implementation, please let us know so we can evaluate pros and cons and switch to it if it's worth it.

@teohhanhui teohhanhui mentioned this pull request Apr 26, 2024
4 tasks
@teohhanhui teohhanhui mentioned this pull request May 7, 2024
4 tasks
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