Fixing kernel commandline parameter validation by phi-nguyendp · Pull Request #148 · rust-vmm/linux-loader

phi-nguyendp · 2023-05-14T17:02:57Z

According to linux kernel document, the value of command line parameter can contains spaces (with double quotes) or equals.

Summary of the PR

Please summarize here why the changes in this PR are needed.

Requirements

Before submitting your PR, please make sure you addressed the following
requirements:

All commits in this PR are signed (with git commit -s), and the commit
message has max 60 characters for the summary and max 75 characters for each
description line.
All added/changed functionality has a corresponding unit/integration
test.
All added/changed public-facing functionality has entries in the "Upcoming
Release" section of CHANGELOG.md (if no such section exists, please create one).
Any newly added unsafe code is properly documented.

roypat · 2023-05-15T09:31:28Z

Heyo, thanks for the contribution. Just a few questions, mainly for my own understanding since I couldn't find the answer in the linux docs >.>

What would happen if the value passed looks like "hello" world", e.g. with a spurious quote in the middle? Do we need to also error out in that case? Also, are single quotes allowed for the purpose of preserving spaces?

From a usability perspective, would it make sense to instead detect if the value the user passes us contains spaces, and add the quotes for them (with maybe some escaping logic for quotes already in the string, although I'm not sure the linux command line allows for escaping them).

phi-nguyendp · 2023-05-16T06:51:25Z

Heyo, thanks for the contribution. Just a few questions, mainly for my own understanding since I couldn't find the answer in the linux docs >.>

What would happen if the value passed looks like "hello" world", e.g. with a spurious quote in the middle? Do we need to also error out in that case? Also, are single quotes allowed for the purpose of preserving spaces?

Only double quotes are allowed. You can't escape " in the value, your example string is invalid.

From a usability perspective, would it make sense to instead detect if the value the user passes us contains spaces, and add the quotes for them (with maybe some escaping logic for quotes already in the string, although I'm not sure the linux command line allows for escaping them).

Maybe a good idea.

roypat · 2023-05-17T09:06:44Z

Heyo, thanks for the contribution. Just a few questions, mainly for my own understanding since I couldn't find the answer in the linux docs >.>
What would happen if the value passed looks like "hello" world", e.g. with a spurious quote in the middle? Do we need to also error out in that case? Also, are single quotes allowed for the purpose of preserving spaces?

Only double quotes are allowed. You can't escape " in the value, your example string is invalid.

I see, thanks for explaining! In that case, we should also validate that no quotes are contained in the middle of values, e.g. if value[1..value.len() - 1].contains('"') {...}

From a usability perspective, would it make sense to instead detect if the value the user passes us contains spaces, and add the quotes for them (with maybe some escaping logic for quotes already in the string, although I'm not sure the linux command line allows for escaping them).

Maybe a good idea.

If we don't have to deal with any complex escaping logic, I think we should do this. Unless I'm missing something, it'd just be

reject strings that contain quotes anywhere but the beginning or end
reject strings with unbalanced quotes
check if string contains spaces.
3.1. If it does, and it is unquoted, add quotes, otherwise leaves as is

What do others think?

andreitraistaru · 2023-05-18T09:46:21Z

Heyo, thanks for the contribution. Just a few questions, mainly for my own understanding since I couldn't find the answer in the linux docs >.>
What would happen if the value passed looks like "hello" world", e.g. with a spurious quote in the middle? Do we need to also error out in that case? Also, are single quotes allowed for the purpose of preserving spaces?

Only double quotes are allowed. You can't escape " in the value, your example string is invalid.

I see, thanks for explaining! In that case, we should also validate that no quotes are contained in the middle of values, e.g. if value[1..value.len() - 1].contains('"') {...}

From a usability perspective, would it make sense to instead detect if the value the user passes us contains spaces, and add the quotes for them (with maybe some escaping logic for quotes already in the string, although I'm not sure the linux command line allows for escaping them).

Maybe a good idea.

If we don't have to deal with any complex escaping logic, I think we should do this. Unless I'm missing something, it'd just be
1. reject strings that contain quotes anywhere but the beginning or end

2. reject strings with unbalanced quotes

3. check if string contains spaces.
   3.1. If it does, and it is unquoted, add quotes, otherwise leaves as is
What do others think?

Thanks @ita93 and @roypat for getting involved into this!

I do agree with the first 2 points (for point 2, we even have a function here that could help us with the implementation).
For point 3, I would agree to do the checking but I'm not quite sure if it would be the right approach to alter the value of the parameter in any way. I mean, if we get a wrong value for a kernel param, I do not consider the Cmdline's responsibility to correct it and store in the Cmdline as it should. For point 3, I would still reject the insertion and that's it. What do you think?

Also, what happens if we receive a parameter's value like "foo1 bar1" "foo2 bar3"? Should we also reject such a string (the case when we get nested double quotes that are balanced)? Not sure from the linux docs, but to me it looks like an invalid value as well...

phi-nguyendp · 2023-05-26T15:19:08Z

Thanks @ita93 and @roypat for getting involved into this!

I do agree with the first 2 points (for point 2, we even have a function here that could help us with the implementation). For point 3, I would agree to do the checking but I'm not quite sure if it would be the right approach to alter the value of the parameter in any way. I mean, if we get a wrong value for a kernel param, I do not consider the Cmdline's responsibility to correct it and store in the Cmdline as it should. For point 3, I would still reject the insertion and that's it. What do you think?

I think that we should follow the purpose of the original function - validating the input, that is all.

Also, what happens if we receive a parameter's value like "foo1 bar1" "foo2 bar3"? Should we also reject such a string (the case when we get nested double quotes that are balanced)? Not sure from the linux docs, but to me it looks like an invalid value as well...

As my understanding, this input may still generate a valid kernel commandline, however, the "foo2 bar3" won't belong to the current key. IMO, we should not accept this kind of input as a value of key=value pair.
We should enforce the value to have two double quotes, one at the start and one at the end of the string if there are any spaces, that is what the kernel expects

andreeaflorescu · 2023-09-05T09:00:40Z

There is no activity on this PR for some time, @ita93 are you still interested in getting this merged? If not, can you please close the PR?

phi-nguyendp · 2024-01-24T10:00:45Z

There is no activity on this PR for some time, @ita93 are you still interested in getting this merged? If not, can you please close the PR?

Sorry for the late reply, I'm still interested in this one. I will try posting it in Slack to ask for a reviewing

roypat · 2024-11-14T13:17:52Z

(replying now since I saw you rebased this recently, sorry for dropping the ball last year!)

Going over the old discussion from last year, I think the only thing left here is to handle the "hello "world" and "foo1 bar1" "foo2 bar3" examples above, and also reject them, right? Also, this should probably get a Changelog entry

rbradford · 2024-11-14T14:45:36Z

You should also rebase your commits to drop the merges.

roypat · 2024-11-14T14:48:58Z

You should also rebase your commits to drop the merges.

I think they're all merge commits created by the github UI (they're all empty), so they'll be dropped if we merge by rebasing

phi-nguyendp · 2024-11-22T06:26:10Z

Going over the old discussion from last year, I think the only thing left here is to handle the "hello "world" and "foo1 bar1" "foo2 bar3" examples above, and also reject them, right? Also, this should probably get a Changelog entry

Hi, I've added a changelog entry in the newest commit.
In summary, the rules will be:

reject strings that contain quotes anywhere but the beginning or end
reject strings that contain spaces but are not quoted.

TimePrinciple

Some thoughts :)

TimePrinciple · 2024-11-22T07:53:56Z

+fn is_quoted(s: &str) -> bool {
+    if let Some(first_char) = s.chars().next() {
+        if let Some(last_char) = s.chars().last() {
+            return first_char == '"' && last_char == '"';
+        }
+    }
+    false
+}


IMHO this implementation would accept input with only a '"' character, should we check if next() and last() actually pointing different location 🤔, or else we check the length of s which should be at least 2 characters long

Hi, I've updated this function as per your suggestion. Can you help me to review it again, please?

roypat

Thanks! Could you squash the last commit such that we don't have commits that modify code introduces in earlier commits though?

According to linux kernel document, the value of command line parameter can contains spaces (with double quotes) or equals. This change will also reject the parameter value that contains quotes in the middle. Signed-off-by: Nguyen Dinh Phi <phind.uet@gmail.com>

rbradford

Rubber stamping after @roypat's review.

phi-nguyendp · 2025-01-20T04:08:01Z

Hi @andreeaflorescu, all checks have passed, could you have a look to see if it can be merged? thank you

roypat · 2025-03-10T09:30:56Z

@ita93 seems like clippy is complaining about something - could you fix that up? I think then we're good to merge this, no one else seems to want to weigh in on it :)

Signed-off-by: Nguyen Dinh Phi <phi.nguyendp@shopee.com>

phi-nguyendp requested review from alxiord, rbradford and sameo as code owners May 14, 2023 17:02

phi-nguyendp force-pushed the fix/value_validation branch from e2610af to 9db1c32 Compare June 4, 2023 03:06

phi-nguyendp requested review from andreitraistaru and roypat as code owners June 4, 2023 03:06

phi-nguyendp force-pushed the fix/value_validation branch 2 times, most recently from c085ab6 to db4d7a4 Compare June 4, 2023 05:26

phi-nguyendp requested a review from JonathanWoollett-Light as a code owner September 6, 2023 03:04

phi-nguyendp requested a review from ShadowCurse as a code owner November 6, 2024 16:08

phi-nguyendp force-pushed the fix/value_validation branch from 1afbee4 to cd4ee10 Compare November 21, 2024 13:04

roypat reviewed Nov 22, 2024

View reviewed changes

Comment thread CHANGELOG.md

TimePrinciple reviewed Nov 22, 2024

View reviewed changes

phi-nguyendp force-pushed the fix/value_validation branch 3 times, most recently from 8f71b30 to ac22037 Compare November 26, 2024 06:47

roypat previously approved these changes Nov 26, 2024

View reviewed changes

phi-nguyendp force-pushed the fix/value_validation branch from ac22037 to 2f1d19a Compare November 27, 2024 08:41

phi-nguyendp dismissed roypat’s stale review via 8a32810 November 27, 2024 08:47

phi-nguyendp force-pushed the fix/value_validation branch 2 times, most recently from f01fb8a to 772ee95 Compare November 27, 2024 09:29

phi-nguyendp force-pushed the fix/value_validation branch from 772ee95 to 2d5e73b Compare November 27, 2024 09:32

roypat previously approved these changes Nov 27, 2024

View reviewed changes

rbradford previously approved these changes Nov 28, 2024

View reviewed changes

Merge branch 'main' into fix/value_validation

c56ed0b

Remove unnecessary return statement

2cecb50

Signed-off-by: Nguyen Dinh Phi <phi.nguyendp@shopee.com>

phi-nguyendp dismissed stale reviews from rbradford and roypat via 2cecb50 March 11, 2025 02:12

roypat approved these changes Mar 11, 2025

View reviewed changes

ShadowCurse approved these changes Mar 11, 2025

View reviewed changes

roypat merged commit f1b5ff6 into rust-vmm:main Mar 11, 2025

Conversation

phi-nguyendp commented May 14, 2023

Summary of the PR

Requirements

Uh oh!

roypat commented May 15, 2023

Uh oh!

phi-nguyendp commented May 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

roypat commented May 17, 2023

Uh oh!

andreitraistaru commented May 18, 2023

Uh oh!

phi-nguyendp commented May 26, 2023

Uh oh!

andreeaflorescu commented Sep 5, 2023

Uh oh!

phi-nguyendp commented Jan 24, 2024

Uh oh!

roypat commented Nov 14, 2024

Uh oh!

rbradford commented Nov 14, 2024

Uh oh!

roypat commented Nov 14, 2024

Uh oh!

phi-nguyendp commented Nov 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

TimePrinciple left a comment

Choose a reason for hiding this comment

Uh oh!

TimePrinciple Nov 22, 2024

Choose a reason for hiding this comment

Uh oh!

phi-nguyendp Nov 27, 2024

Choose a reason for hiding this comment

Uh oh!

roypat left a comment

Choose a reason for hiding this comment

Uh oh!

rbradford left a comment

Choose a reason for hiding this comment

Uh oh!

phi-nguyendp commented Jan 20, 2025

Uh oh!

roypat commented Mar 10, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

phi-nguyendp commented May 16, 2023 •

edited

Loading

phi-nguyendp commented Nov 22, 2024 •

edited

Loading