linux-kernel - Re: [PATCH 1/3] lib/test_kmod: tidy up bounds checking

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20170801231021.GY18884@wotan.suse.de>
Date:   Wed, 2 Aug 2017 01:10:21 +0200
From:   "Luis R. Rodriguez" <mcgrof@...nel.org>
To:     Dan Carpenter <dan.carpenter@...cle.com>
Cc:     "Luis R. Rodriguez" <mcgrof@...nel.org>,
        linux-kernel@...r.kernel.org, kernel-janitors@...r.kernel.org
Subject: Re: [PATCH 1/3] lib/test_kmod: tidy up bounds checking

On Fri, Jul 07, 2017 at 11:39:33AM +0300, Dan Carpenter wrote:
> There is technically a bug where we don't test for negatives in
> test_dev_config_update_uint_sync().  "new" is long and UINT_MAX is
> unsigned int so on 64 bit systems negatives are allowed.

Good catch. I however prefer we instead just replace kstrtol() with
kstrtoul() in both of these cases as an atomic separate patch with
a Fixes tag. I'll do this and roll in your other patches. Below is
what I mean as one atomic fix.

diff --git a/lib/test_kmod.c b/lib/test_kmod.c
index 90c91541fc16..8fc0a7a19c83 100644
--- a/lib/test_kmod.c
+++ b/lib/test_kmod.c
@@ -880,10 +880,10 @@ static int test_dev_config_update_uint_sync(struct kmod_test_device *test_dev,
 					    int (*test_sync)(struct kmod_test_device *test_dev))
 {
 	int ret;
-	long new;
+	unsigned long new;
 	unsigned int old_val;
 
-	ret = kstrtol(buf, 10, &new);
+	ret = kstrtoul(buf, 10, &new);
 	if (ret)
 		return ret;
 
@@ -918,9 +918,9 @@ static int test_dev_config_update_uint_range(struct kmod_test_device *test_dev,
 					     unsigned int max)
 {
 	int ret;
-	long new;
+	unsigned long new;
 
-	ret = kstrtol(buf, 10, &new);
+	ret = kstrtoul(buf, 10, &new);
 	if (ret)
 		return ret;
 
> In the next test I removed the UINT_MAX comparison because "max" is
> already an unsigned int so we already know that "new" can't be larger
> than UINT_MAX.

Makes sense, will roll this in as a separate patch on your part.

> On the third test, I just flipped the tests around so we consistently
> test the lower bound before the upper bound.

And since non-functional I will also split up into another patch.

  Luis