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

ethtool ioctl crashes if number of strings exceeds hardcoded value #1112

Open
pspacek opened this issue Jul 12, 2023 · 5 comments
Open

ethtool ioctl crashes if number of strings exceeds hardcoded value #1112

pspacek opened this issue Jul 12, 2023 · 5 comments
Labels

Comments

@pspacek
Copy link
Contributor

pspacek commented Jul 12, 2023

Version affected: c7da937

Problematic code:
https://github.com/svinota/pyroute2/blob/c7da937999dc59167f81f9466c80e891fb7be736/pyroute2/ethtool/ioctl.py#L284C14-L284C14

It turns out that comment

        # If you have more than 256 features on your NIC
        # they will not be seen by it

... is not correct. The code outright segfaults if number of strings returned exceeds hardcoded value. Easiest way to test is to manually drop the number to a small value, but it is practical problem e.g. in AWS VM type c5n.18xlarge which can return 1105 strings.

We already know the correct values from ioctl ETHTOOL_GSSET_INFO but it is not used in structure definition or checked anywhere. I might try to fix it but it will require redefining array sizes dynamically, I think.

@svinota svinota added the bug label Jul 13, 2023
@svinota
Copy link
Owner

svinota commented Jul 13, 2023

Oh. Thanks for the report. If you try to provide a PR it would be nice, otherwise I'm to address this bug.

@pspacek
Copy link
Contributor Author

pspacek commented Jul 14, 2023

Turns out I did not have time for it - I can get to it in couple weeks. If you are faster so be it ...

@svinota
Copy link
Owner

svinota commented Jul 17, 2023

Reproduced.

I will try to fix the issue this week.

@svinota
Copy link
Owner

svinota commented Aug 11, 2023

It took some more time than expected.

Could you please check if it works for you?

@pspacek
Copy link
Contributor Author

pspacek commented Aug 24, 2023

This fix partially works in a sense that it does not crash if you ask for the same stringset every time. It can potentially crash when you change set_id on the fly. I've attempted to generalize this in #1126 . It makes the structures a bit less dynamic but correctly takes into account different string set sizes for distinct IDs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants