After upgrading to linux-2.4.13-ac5, everything seems to work, except all
my vtun tunnels.
a _lot_ of searching revealed this code fragment:
/*
* Verify the string as this thing may have come from
* the user. There must be one "%d" and no other "%"
* characters.
*/
p = strchr(name, '%');
if (!p || p[1] != 'd' || strchr(p+2, '%'))
return -EINVAL;
Well, obviously my devicename _do_ come "from the user", as I really like
to name my tun devices (and everything else). The problem is that vtund
passes in "tun2" as devicename, which does not contain a "%d".
Maybe this piece of code is designed to fix security problems, but it
keeps vtund from working properly.
How about this change?
- if (!p || p[1] != 'd' || strchr(p+2, '%'))
+ if (p && (p[1] != 'd' || strchr(p+2, '%')))
--
-----==- |
----==-- _ |
---==---(_)__ __ ____ __ Marc Lehmann +--
--==---/ / _ \/ // /\ \/ / [email protected] |e|
-=====/_/_//_/\_,_/ /_/\_\ XX11-RIPE --+
The choice of a GNU generation |
|
On Tue, Oct 30, 2001 at 02:17:40AM +0100, " Marc A. Lehmann " <[email protected]> wrote:
> a _lot_ of searching revealed this code fragment:
In my usual attempt to generate more traffic, I forgot to mention that I
found it in net/core/dev.c ;)
(oh, and after reading the comments int hat file, I think that maybe tun.c
simply shouldn't call dev_alloc_name...)
--
-----==- |
----==-- _ |
---==---(_)__ __ ____ __ Marc Lehmann +--
--==---/ / _ \/ // /\ \/ / [email protected] |e|
-=====/_/_//_/\_,_/ /_/\_\ XX11-RIPE --+
The choice of a GNU generation |
|
>On Tue, Oct 30, 2001 at 02:17:40AM +0100, " Marc A. Lehmann " <[email protected]> wrote:
>> a _lot_ of searching revealed this code fragment:
>
>In my usual attempt to generate more traffic, I forgot to mention that I
>found it in net/core/dev.c ;)
>
>(oh, and after reading the comments int hat file, I think that maybe tun.c
>simply shouldn't call dev_alloc_name...)
Hmm, let me check that.
I was under the assumption that it's dev.c bug :)
Max
From: Maksim Krasnyanskiy <[email protected]>
Date: Mon, 29 Oct 2001 17:48:35 -0800
>On Tue, Oct 30, 2001 at 02:17:40AM +0100, " Marc A. Lehmann " <[email protected]> wrote:
>> a _lot_ of searching revealed this code fragment:
>
>In my usual attempt to generate more traffic, I forgot to mention that I
>found it in net/core/dev.c ;)
>
>(oh, and after reading the comments int hat file, I think that maybe tun.c
>simply shouldn't call dev_alloc_name...)
Hmm, let me check that.
I was under the assumption that it's dev.c bug :)
Basically, don't pass a string lack one "%d" into dev_alloc_name
because dev_alloc_name() runs sprintf on that string with an
integer argument.
Franks a lot,
David S. Miller
[email protected]
On Mon, Oct 29, 2001 at 05:53:12PM -0800, "David S. Miller" <[email protected]> wrote:
> Basically, don't pass a string lack one "%d" into dev_alloc_name
> because dev_alloc_name() runs sprintf on that string with an
> integer argument.
I fail to follow you - yes, dev_alloc_name calls sprintf on it, but
sprintf works fine on strings without "%d", and dev_alloc_name also works
fine (despite a little suboptimal).
On Mon, Oct 29, 2001 at 05:48:35PM -0800, Maksim Krasnyanskiy <[email protected]> wrote:
> >(oh, and after reading the comments int hat file, I think that maybe tun.c
> >simply shouldn't call dev_alloc_name...)
> Hmm, let me check that.
> I was under the assumption that it's dev.c bug :)
well, reading the part again it seems that dev_alloc_name is not "allocating
a name" but just looking for a free one. If it is indeed logically allocating
the devicename then it's a bug in dev.c. If it is just used to find a free
existing name, then it's a bug in tun.c (and elsewhere), in that it simply
shouldn't call dev_alloc_name, but instead allocates the name itself.
my personal opinion is that dev_alloc_name should work, as it would be the
logical place to do this stuff, an abstraction. it could be coded more
efficiently, but it doesn't seem to be a terrible important place anyways.
but I also must admit that I, well, know nothing ;)
--
-----==- |
----==-- _ |
---==---(_)__ __ ____ __ Marc Lehmann +--
--==---/ / _ \/ // /\ \/ / [email protected] |e|
-=====/_/_//_/\_,_/ /_/\_\ XX11-RIPE --+
The choice of a GNU generation |
|
From: <[email protected] ( Marc) (A.) (Lehmann )>
Date: Wed, 31 Oct 2001 01:05:00 +0100
On Mon, Oct 29, 2001 at 05:53:12PM -0800, "David S. Miller" <[email protected]> wrote:
> Basically, don't pass a string lack one "%d" into dev_alloc_name
> because dev_alloc_name() runs sprintf on that string with an
> integer argument.
I fail to follow you - yes, dev_alloc_name calls sprintf on it, but
sprintf works fine on strings without "%d", and dev_alloc_name also works
fine (despite a little suboptimal).
You're right, it should allow the "string has no '%' at all" case
as well. Please, someone send me a patch which does this.
Franks a lot,
David S. Miller
[email protected]
On Wed, Oct 31, 2001 at 12:30:56AM -0800, "David S. Miller" <[email protected]> wrote:
> You're right, it should allow the "string has no '%' at all" case
> as well. Please, someone send me a patch which does this.
My original mail contained a one-line fix, suboptimal but works fine for me.
I also found a more elaborate patch:
http://www.geocrawler.com/lists/3/SourceForge/12162/0/6896612/
it seems to use a fancier algorithm and has been used by more people.
--
-----==- |
----==-- _ |
---==---(_)__ __ ____ __ Marc Lehmann +--
--==---/ / _ \/ // /\ \/ / [email protected] |e|
-=====/_/_//_/\_,_/ /_/\_\ XX11-RIPE --+
The choice of a GNU generation |
|
At 10:43 AM 10/31/01 +0100, [email protected] ( Marc) (A.) (Lehmann ) wrote:
>On Wed, Oct 31, 2001 at 12:30:56AM -0800, "David S. Miller" <[email protected]> wrote:
>> You're right, it should allow the "string has no '%' at all" case
>> as well. Please, someone send me a patch which does this.
>
>My original mail contained a one-line fix, suboptimal but works fine for me.
>I also found a more elaborate patch:
>
> http://www.geocrawler.com/lists/3/SourceForge/12162/0/6896612/
>
>it seems to use a fancier algorithm and has been used by more people.
Here is the patch for TUN/TAP to remove that suboptimality :).
So we won't call dev_alloc_name if name is not a template.
--- tun.c.old Tue Oct 30 21:00:55 2001
+++ tun.c Tue Oct 30 21:10:17 2001
@@ -377,8 +377,11 @@
if (*ifr->ifr_name)
name = ifr->ifr_name;
- if ((err = dev_alloc_name(&tun->dev, name)) < 0)
- goto failed;
+ if (strchr(name, '%')) {
+ err = dev_alloc_name(&tun->dev, name);
+ if (err) goto failed;
+ }
+
if ((err = register_netdevice(&tun->dev)))
goto failed;
Untested but looks obvious.
Max
From: Maksim Krasnyanskiy <[email protected]>
Date: Wed, 31 Oct 2001 09:55:47 -0800
Here is the patch for TUN/TAP to remove that suboptimality :).
So we won't call dev_alloc_name if name is not a template.
This won't work. The whole purpose of calling dev_alloc_name
is to twofold:
1) Make sure the name string is unique
2) Copy that name into dev->name if it is unique
I'm going to change dev_alloc_name() to allow non-'%' names
instead, that is a better fix.
Franks a lot,
David S. Miller
[email protected]
> Here is the patch for TUN/TAP to remove that suboptimality :).
> So we won't call dev_alloc_name if name is not a template.
>
>This won't work. The whole purpose of calling dev_alloc_name is to twofold:
>
>1) Make sure the name string is unique
>
>2) Copy that name into dev->name if it is unique
>
>I'm going to change dev_alloc_name() to allow non-'%' names instead, that is a better fix.
Ok with me. I new that it's a dev.c bug from the beginning ;-)
Max