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.

Sign up for GitHub

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

Jump to bottom

Fix the bug that config->env is greater than ulong_max when units->val =1 #130

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

Fix the bug that config->env is greater than ulong_max when units->val =1 #130

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions ras-page-isolation.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -164,6 +164,17 @@ static void parse_isolation_env(struct isolation *config)
value *= units->val;
if (tmp != 0 && value / tmp != units->val)
config->overflow = true;
/**
* if units->val is 1, config->env is greater than ulong_max, so it is can strtoul
* if failed, the value is greater than ulong_max, set config->overflow = true
*/
if (units->val == 1) {
char *endptr;
unsigned long converted_value = strtoul(config->env, &endptr, 10);
if (errno == ERANGE || *endptr != '\0')
config->overflow = true;
}
unit_matched = 0;
Copy link
Owner

@mchehab mchehab Jan 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

The logic seems weird to me. First of all, units is an array. It may potentially have units->name == NULL as the first element, indicating the end of such array, in which case units->val should not be used.

Also, I tried to understand why units->val==1 so special. I mean, why other values can't overflow?

Looking at the check code:

       /* if env value string is greater than ulong_max, truncate the last digit */
        sscanf(config->env, "%lu", &value);
        for (units = config->units; units->name; units++) {
                if (!strcasecmp(config->unit, units->name))
                        unit_matched = 1;
                if (unit_matched) {
                        tmp = value;
                        value *= units->val;
                        if (tmp != 0 && value / tmp != units->val)
                                config->overflow = true;
                }
        }

The idea of the above check were exactly to catch ERANGE issues. If it is broken, or should instead use strtoul to catch overflows, please fix it or replace the check by a better logic.

Copy link
Author

Choose a reason for hiding this comment

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

if units->val==1 and PAGE_CE_THRESHOLD or PAGE_CE_REFRESH_CYCLE is greater than ulong_max.
e.g.
PAGE_CE_THRESHOLD=9999999999999999999999999999999999999999999999999
PAGE_CE_REFRESH_CYCLE=333333999999999999999999999999999999999s

/* if env value string is greater than ulong_max, truncate the last digit */
	sscanf(config->env, "%lu", &value);
	for (units = config->units; units->name; units++) {
		if (!strcasecmp(config->unit, units->name))
			unit_matched = 1;
		if (unit_matched) {
			tmp = value;
			value *= units->val;
			if (tmp != 0 && value / tmp != units->val)
				config->overflow = true;
		}
	}

then sscanf(config->env, "%lu", &value); value is 18446744073709551615(2^64-1)
value *= units->val; // value is 2^64-1, because units->val == 1
if (tmp != 0 && value / tmp != units->val)
and then that condition is not met. because (value / tmp == 1)

but units->val is not 1, e.g. 1000,
then sscanf(config->env, "%lu", &value); value is 18446744073709551615(2^64-1)
value *= units->val; // value is 2^64-1, Even though value is multiplied by 1000, the maximum value is still 2^64-1
if (tmp != 0 && value / tmp != units->val)
and then that condition is met. because (value / tmp == 1, not 1000)

}
}
config->val = value;
Expand Down