On Wed, 5 Jan 2011 09:43:57 +0100
Michal Hocko <mhocko@xxxxxxx> wrote:
> ...
>
> proc_doulongvec_minmax may fail if the given buffer doesn't represent
> a valid number. If we provide something invalid we will initialize the
> resulting value (nr_overcommit_huge_pages in this case) to a random
> value from the stack.
>
> The issue was introduced by a3d0c6aa when the default handler has been
> replaced by the helper function where we do not check the return value.
>
> Reproducer:
> echo "" > /proc/sys/vm/nr_overcommit_hugepages
>
> ...
>
> --- a/mm/hugetlb.c
> +++ b/mm/hugetlb.c
> @@ -1928,7 +1928,8 @@ static int hugetlb_sysctl_handler_common(bool obey_mempolicy,
>
> table->data = &tmp;
> table->maxlen = sizeof(unsigned long);
> - proc_doulongvec_minmax(table, write, buffer, length, ppos);
> + if (proc_doulongvec_minmax(table, write, buffer, length, ppos))
> + return -EINVAL;
proc_doulongvec_minmax() can return -EFAULT or -ENOMEM. It is
incorrect to unconditionally convert those into -EINVAL.
> if (write) {
> NODEMASK_ALLOC(nodemask_t, nodes_allowed,
hm, the code doesn't check that NODEMASK_ALLOC succeeded. That
NODEMASK_ALLOC conversion was quite sloppy.
--- a/mm/hugetlb.c~hugetlb-check-the-return-value-of-string-conversion-in-sysctl-handler-fix
+++ a/mm/hugetlb.c
@@ -1859,14 +1859,16 @@ static int hugetlb_sysctl_handler_common
{
struct hstate *h = &default_hstate;
unsigned long tmp;
+ int ret;
if (!write)
tmp = h->max_huge_pages;
table->data = &tmp;
table->maxlen = sizeof(unsigned long);
- if (proc_doulongvec_minmax(table, write, buffer, length, ppos))
- return -EINVAL;
+ ret = proc_doulongvec_minmax(table, write, buffer, length, ppos);
+ if (ret)
+ goto out;
if (write) {
NODEMASK_ALLOC(nodemask_t, nodes_allowed,
@@ -1881,8 +1883,8 @@ static int hugetlb_sysctl_handler_common
if (nodes_allowed != &node_states[N_HIGH_MEMORY])
NODEMASK_FREE(nodes_allowed);
}
-
- return 0;
+out:
+ return ret;
}
int hugetlb_sysctl_handler(struct ctl_table *table, int write,
@@ -1920,22 +1922,24 @@ int hugetlb_overcommit_handler(struct ct
{
struct hstate *h = &default_hstate;
unsigned long tmp;
+ int ret;
if (!write)
tmp = h->nr_overcommit_huge_pages;
table->data = &tmp;
table->maxlen = sizeof(unsigned long);
- if (proc_doulongvec_minmax(table, write, buffer, length, ppos))
- return -EINVAL;
+ ret = proc_doulongvec_minmax(table, write, buffer, length, ppos);
+ if (ret)
+ goto out;
if (write) {
spin_lock(&hugetlb_lock);
h->nr_overcommit_huge_pages = tmp;
spin_unlock(&hugetlb_lock);
}
-
- return 0;
+out:
+ return ret;
}
#endif /* CONFIG_SYSCTL */
_
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@xxxxxxxxxx For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom policy in Canada: sign http://dissolvethecrtc.ca/
Don't email: <a href=mailto:"dont@xxxxxxxxx";> email@xxxxxxxxx </a>