This makes the size argument a const, since it is always populated by
the caller. Additionally double-checks to make sure the copy_from_user
can never overflow, keeping CONFIG_DEBUG_STRICT_USER_COPY_CHECKS happy:
In function 'copy_from_user',
inlined from '__tun_chr_ioctl' at drivers/net/tun.c:1871:7:
... copy_from_user() buffer size is not provably correct
Signed-off-by: Kees Cook <redacted>
---
drivers/net/tun.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
From: David Laight <hidden> Date: 2014-09-29 11:05:47
From: Kees Cook
This makes the size argument a const, since it is always populated by
the caller.
There is almost no point making parameters 'const.
('const foo *' makes sense).
Additionally double-checks to make sure the copy_from_user
can never overflow, keeping CONFIG_DEBUG_STRICT_USER_COPY_CHECKS happy:
In function 'copy_from_user',
inlined from '__tun_chr_ioctl' at drivers/net/tun.c:1871:7:
... copy_from_user() buffer size is not provably correct
If 'ifreq_len' could be too big then you want to error the ioctl, not panic.
If it can't be too big you don't need the check.
David
@@ -1869,6 +1869,7 @@ static long __tun_chr_ioctl(struct file *file, unsigned int cmd,intret;if(cmd==TUNSETIFF||cmd==TUNSETQUEUE||_IOC_TYPE(cmd)==0x89){+BUG_ON(ifreq_len>sizeof(ifr));if(copy_from_user(&ifr,argp,ifreq_len))return-EFAULT;}else{--
1.9.1
--
Kees Cook
Chrome OS Security
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
From: "Michael S. Tsirkin" <mst@redhat.com> Date: 2014-09-29 11:45:33
On Sun, Sep 28, 2014 at 04:27:53PM -0700, Kees Cook wrote:
This makes the size argument a const, since it is always populated by
the caller. Additionally double-checks to make sure the copy_from_user
can never overflow, keeping CONFIG_DEBUG_STRICT_USER_COPY_CHECKS happy:
In function 'copy_from_user',
inlined from '__tun_chr_ioctl' at drivers/net/tun.c:1871:7:
... copy_from_user() buffer size is not provably correct
Signed-off-by: Kees Cook <redacted>
What exactly is the issue here?
__tun_chr_ioctl is called with sizeof(struct compat_ifreq)
or sizeof (struct ifreq) as the last argument.
So this looks like a false positive, but
CONFIG_DEBUG_STRICT_USER_COPY_CHECKS machinery is supposed
to avoid false positives.
On which architecture is this?
From: "Michael S. Tsirkin" <mst@redhat.com> Date: 2014-09-29 11:59:14
On Mon, Sep 29, 2014 at 02:48:49PM +0300, Michael S. Tsirkin wrote:
On Sun, Sep 28, 2014 at 04:27:53PM -0700, Kees Cook wrote:
quoted
This makes the size argument a const, since it is always populated by
the caller. Additionally double-checks to make sure the copy_from_user
can never overflow, keeping CONFIG_DEBUG_STRICT_USER_COPY_CHECKS happy:
In function 'copy_from_user',
inlined from '__tun_chr_ioctl' at drivers/net/tun.c:1871:7:
... copy_from_user() buffer size is not provably correct
Signed-off-by: Kees Cook <redacted>
What exactly is the issue here?
__tun_chr_ioctl is called with sizeof(struct compat_ifreq)
or sizeof (struct ifreq) as the last argument.
So this looks like a false positive, but
CONFIG_DEBUG_STRICT_USER_COPY_CHECKS machinery is supposed
to avoid false positives.
On which architecture is this?
Also - which kernel?
Does your kernel include: commit 3df7b41aa5e7797f391d0a41f8b0dce1fe366a09
x86: Unify copy_from_user() size checking
?
On Mon, Sep 29, 2014 at 4:04 AM, David Laight [off-list ref] wrote:
From: Kees Cook
quoted
This makes the size argument a const, since it is always populated by
the caller.
There is almost no point making parameters 'const.
('const foo *' makes sense).
quoted
Additionally double-checks to make sure the copy_from_user
can never overflow, keeping CONFIG_DEBUG_STRICT_USER_COPY_CHECKS happy:
In function 'copy_from_user',
inlined from '__tun_chr_ioctl' at drivers/net/tun.c:1871:7:
... copy_from_user() buffer size is not provably correct
If 'ifreq_len' could be too big then you want to error the ioctl, not panic.
If it can't be too big you don't need the check.
The ifreq_len comes from the callers, and is the output of "sizeof"
which is const. Changing the function parameter to "const" means any
changes made in the future where the incoming value isn't const, the
compiler will throw a warning.
-Kees
--
Kees Cook
Chrome OS Security
On Mon, Sep 29, 2014 at 4:48 AM, Michael S. Tsirkin [off-list ref] wrote:
On Sun, Sep 28, 2014 at 04:27:53PM -0700, Kees Cook wrote:
quoted
This makes the size argument a const, since it is always populated by
the caller. Additionally double-checks to make sure the copy_from_user
can never overflow, keeping CONFIG_DEBUG_STRICT_USER_COPY_CHECKS happy:
In function 'copy_from_user',
inlined from '__tun_chr_ioctl' at drivers/net/tun.c:1871:7:
... copy_from_user() buffer size is not provably correct
Signed-off-by: Kees Cook <redacted>
What exactly is the issue here?
__tun_chr_ioctl is called with sizeof(struct compat_ifreq)
or sizeof (struct ifreq) as the last argument.
Correct. There is no vulnerability here; I am attempting to both make
the code more defensive to future changes, and to keep
CONFIG_DEBUG_STRICT_USER_COPY_CHECKS happy.
So this looks like a false positive, but
CONFIG_DEBUG_STRICT_USER_COPY_CHECKS machinery is supposed
to avoid false positives.
The support in GCC is currently a bit faulty, and it seems that it
didn't notice the two callers were static values, so instead, adding
an explicit test keeps it happy.
From: Hannes Frederic Sowa <hidden> Date: 2014-09-29 20:05:14
On Mo, 2014-09-29 at 12:41 -0700, Kees Cook wrote:
On Mon, Sep 29, 2014 at 4:04 AM, David Laight [off-list ref] wrote:
quoted
From: Kees Cook
quoted
This makes the size argument a const, since it is always populated by
the caller.
There is almost no point making parameters 'const.
('const foo *' makes sense).
quoted
Additionally double-checks to make sure the copy_from_user
can never overflow, keeping CONFIG_DEBUG_STRICT_USER_COPY_CHECKS happy:
In function 'copy_from_user',
inlined from '__tun_chr_ioctl' at drivers/net/tun.c:1871:7:
... copy_from_user() buffer size is not provably correct
If 'ifreq_len' could be too big then you want to error the ioctl, not panic.
If it can't be too big you don't need the check.
The ifreq_len comes from the callers, and is the output of "sizeof"
which is const. Changing the function parameter to "const" means any
changes made in the future where the incoming value isn't const, the
compiler will throw a warning.
Hmmm, I think you want something like BUILD_BUG_ON(!
__builtin_constant_p(var)). const in function argument only ensures that
the value cannot be modified in the function.
Bye,
Hannes
From: David Laight <hidden> Date: 2014-09-30 08:22:19
From: Hannes Frederic
On Mo, 2014-09-29 at 12:41 -0700, Kees Cook wrote:
quoted
On Mon, Sep 29, 2014 at 4:04 AM, David Laight [off-list ref] wrote:
quoted
From: Kees Cook
quoted
This makes the size argument a const, since it is always populated by
the caller.
There is almost no point making parameters 'const.
('const foo *' makes sense).
quoted
Additionally double-checks to make sure the copy_from_user
can never overflow, keeping CONFIG_DEBUG_STRICT_USER_COPY_CHECKS happy:
In function 'copy_from_user',
inlined from '__tun_chr_ioctl' at drivers/net/tun.c:1871:7:
... copy_from_user() buffer size is not provably correct
If 'ifreq_len' could be too big then you want to error the ioctl, not panic.
If it can't be too big you don't need the check.
The ifreq_len comes from the callers, and is the output of "sizeof"
which is const. Changing the function parameter to "const" means any
changes made in the future where the incoming value isn't const, the
compiler will throw a warning.
Hmmm, I think you want something like BUILD_BUG_ON(!
__builtin_constant_p(var)). const in function argument only ensures that
the value cannot be modified in the function.
You'd have to do something in the header file - nothing in the function
body can do that check.
I've not got the source handy, but in this case is there any need to
actually pass the size at all?
Is it always the same fixed constant??
David
From: Hannes Frederic Sowa <hidden> Date: 2014-09-30 11:04:00
On Di, 2014-09-30 at 08:20 +0000, David Laight wrote:
From: Hannes Frederic
quoted
On Mo, 2014-09-29 at 12:41 -0700, Kees Cook wrote:
quoted
On Mon, Sep 29, 2014 at 4:04 AM, David Laight [off-list ref] wrote:
quoted
From: Kees Cook
quoted
This makes the size argument a const, since it is always populated by
the caller.
There is almost no point making parameters 'const.
('const foo *' makes sense).
quoted
Additionally double-checks to make sure the copy_from_user
can never overflow, keeping CONFIG_DEBUG_STRICT_USER_COPY_CHECKS happy:
In function 'copy_from_user',
inlined from '__tun_chr_ioctl' at drivers/net/tun.c:1871:7:
... copy_from_user() buffer size is not provably correct
If 'ifreq_len' could be too big then you want to error the ioctl, not panic.
If it can't be too big you don't need the check.
The ifreq_len comes from the callers, and is the output of "sizeof"
which is const. Changing the function parameter to "const" means any
changes made in the future where the incoming value isn't const, the
compiler will throw a warning.
Hmmm, I think you want something like BUILD_BUG_ON(!
__builtin_constant_p(var)). const in function argument only ensures that
the value cannot be modified in the function.
You'd have to do something in the header file - nothing in the function
body can do that check.
Sure, it should work. You only need to make sure that gcc inlines the
function, so the value is constant (it is not enough that gcc knows the
value range, one specific constant is needed). So the simplest fix for
this is to specify __tun_chr_ioctl as __always_inline. ;)
Bye,
Hannes
From: David Laight <hidden> Date: 2014-09-30 11:20:41
From: Hannes Frederic
On Di, 2014-09-30 at 08:20 +0000, David Laight wrote:
quoted
From: Hannes Frederic
quoted
On Mo, 2014-09-29 at 12:41 -0700, Kees Cook wrote:
quoted
On Mon, Sep 29, 2014 at 4:04 AM, David Laight [off-list ref] wrote:
quoted
From: Kees Cook
quoted
This makes the size argument a const, since it is always populated by
the caller.
There is almost no point making parameters 'const.
('const foo *' makes sense).
quoted
Additionally double-checks to make sure the copy_from_user
can never overflow, keeping CONFIG_DEBUG_STRICT_USER_COPY_CHECKS happy:
In function 'copy_from_user',
inlined from '__tun_chr_ioctl' at drivers/net/tun.c:1871:7:
... copy_from_user() buffer size is not provably correct
If 'ifreq_len' could be too big then you want to error the ioctl, not panic.
If it can't be too big you don't need the check.
The ifreq_len comes from the callers, and is the output of "sizeof"
which is const. Changing the function parameter to "const" means any
changes made in the future where the incoming value isn't const, the
compiler will throw a warning.
Hmmm, I think you want something like BUILD_BUG_ON(!
__builtin_constant_p(var)). const in function argument only ensures that
the value cannot be modified in the function.
You'd have to do something in the header file - nothing in the function
body can do that check.
Sure, it should work. You only need to make sure that gcc inlines the
function, so the value is constant (it is not enough that gcc knows the
value range, one specific constant is needed). So the simplest fix for
this is to specify __tun_chr_ioctl as __always_inline. ;)
You are joking aren't you???
Look at the code.
I'd suggest fixing whatever implements CONFIG_DEBUG_STRICT_USER_COPY_CHECKS
to be less picky.
David
From: "Michael S. Tsirkin" <mst@redhat.com> Date: 2014-09-30 11:26:05
On Mon, Sep 29, 2014 at 12:48:47PM -0700, Kees Cook wrote:
On Mon, Sep 29, 2014 at 4:48 AM, Michael S. Tsirkin [off-list ref] wrote:
quoted
On Sun, Sep 28, 2014 at 04:27:53PM -0700, Kees Cook wrote:
quoted
This makes the size argument a const, since it is always populated by
the caller. Additionally double-checks to make sure the copy_from_user
can never overflow, keeping CONFIG_DEBUG_STRICT_USER_COPY_CHECKS happy:
In function 'copy_from_user',
inlined from '__tun_chr_ioctl' at drivers/net/tun.c:1871:7:
... copy_from_user() buffer size is not provably correct
Signed-off-by: Kees Cook <redacted>
What exactly is the issue here?
__tun_chr_ioctl is called with sizeof(struct compat_ifreq)
or sizeof (struct ifreq) as the last argument.
Correct. There is no vulnerability here; I am attempting to both make
the code more defensive to future changes, and to keep
CONFIG_DEBUG_STRICT_USER_COPY_CHECKS happy.
quoted
So this looks like a false positive, but
CONFIG_DEBUG_STRICT_USER_COPY_CHECKS machinery is supposed
to avoid false positives.
The support in GCC is currently a bit faulty, and it seems that it
didn't notice the two callers were static values, so instead, adding
an explicit test keeps it happy.
quoted
On which architecture is this?
This is on x86, but with CONFIG_DEBUG_STRICT_USER_COPY_CHECKS
correctly enabled (gcc after 4.6 broke its ability to correctly
optimize), which I've been playing with trying to get gcc working
again. I sent the patch because it seems like it's a reasonable
defensive change to make.
For the BUG_ON - I guess it's reasonable but we'll just break it again
by random code changes in the future. And when we do, I don't cherish
the thought of using trial and error to find a way to shut up the
warning again.
If gcc can't be fixed, something explicit like the uninitialized_var
is needed.
For const as an argument - is it also about gcc bugs?
We don't usually do this in kernel so if it's useful,
it has to be documented.
Finally, why are you switching the type from int to size_t?
Pls document the reason in the commit log.
I think what you are trying to do is really useful, but
I doubt making random code changes that happen to make a
specific gcc version happy is maintainable.
HTH
Thanks!
From: Hannes Frederic Sowa <hidden> Date: 2014-09-30 12:04:17
On Di, 2014-09-30 at 11:18 +0000, David Laight wrote:
From: Hannes Frederic
quoted
On Di, 2014-09-30 at 08:20 +0000, David Laight wrote:
quoted
From: Hannes Frederic
quoted
On Mo, 2014-09-29 at 12:41 -0700, Kees Cook wrote:
quoted
On Mon, Sep 29, 2014 at 4:04 AM, David Laight [off-list ref] wrote:
quoted
From: Kees Cook
quoted
This makes the size argument a const, since it is always populated by
the caller.
There is almost no point making parameters 'const.
('const foo *' makes sense).
quoted
Additionally double-checks to make sure the copy_from_user
can never overflow, keeping CONFIG_DEBUG_STRICT_USER_COPY_CHECKS happy:
In function 'copy_from_user',
inlined from '__tun_chr_ioctl' at drivers/net/tun.c:1871:7:
... copy_from_user() buffer size is not provably correct
If 'ifreq_len' could be too big then you want to error the ioctl, not panic.
If it can't be too big you don't need the check.
The ifreq_len comes from the callers, and is the output of "sizeof"
which is const. Changing the function parameter to "const" means any
changes made in the future where the incoming value isn't const, the
compiler will throw a warning.
Hmmm, I think you want something like BUILD_BUG_ON(!
__builtin_constant_p(var)). const in function argument only ensures that
the value cannot be modified in the function.
You'd have to do something in the header file - nothing in the function
body can do that check.
Sure, it should work. You only need to make sure that gcc inlines the
function, so the value is constant (it is not enough that gcc knows the
value range, one specific constant is needed). So the simplest fix for
this is to specify __tun_chr_ioctl as __always_inline. ;)
You are joking aren't you???
It would propagate the value from the compat and non-compat helper
functions to __tun_chr_ioctl and the __builtin_constant_p checks would
be true in the inlined copy_from/to_user, so no, I was not joking, but
because of my smiley I didn't considered this fix seriously.
Look at the code.
I did.
I'd suggest fixing whatever implements CONFIG_DEBUG_STRICT_USER_COPY_CHECKS
to be less picky.
This might involve gcc changes. E.g. on my gcc I don't get any errors.
But by looking at the code it might be plausible wrong estimations are
being made.
But somehow I really cannot follow the original bug report. Looks like
Kees' __builtin_object_size is broken?
I also checked my tun.i file, I really use gcc __builtin_object_size.
Strange...
Bye,
Hannes