2.4.13-ac5 && vtun not working

2.4.13-ac5 && vtun not working

[Lehmann]

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      +--
      --==---/ / _ \/ // /\ \/ /       pcg@goof.com      |e|
      -=====/_/_//_/\_,_/ /_/\_\       XX11-RIPE         --+
    The choice of a GNU generation                       |
                                                         |

Re: 2.4.13-ac5 && vtun not working

[Lehmann]

On Tue, Oct 30, 2001 at 02:17:40AM +0100, " Marc A. Lehmann " <pcg@goof.com> 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      +--
      --==---/ / _ \/ // /\ \/ /       pcg@goof.com      |e|
      -=====/_/_//_/\_,_/ /_/\_\       XX11-RIPE         --+
    The choice of a GNU generation                       |
                                                         |

Re: 2.4.13-ac5 && vtun not working

[Maksim Krasnyanskiy]

>On Tue, Oct 30, 2001 at 02:17:40AM +0100, " Marc A. Lehmann " <pcg@goof.com> 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

Re: 2.4.13-ac5 && vtun not working

[David S. Miller]

   From: Maksim Krasnyanskiy <maxk@qualcomm.com>
   Date: Mon, 29 Oct 2001 17:48:35 -0800

   >On Tue, Oct 30, 2001 at 02:17:40AM +0100, " Marc A. Lehmann " <pcg@goof.com> 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
davem@redhat.com

Re: 2.4.13-ac5 && vtun not working

[Lehmann]

On Mon, Oct 29, 2001 at 05:53:12PM -0800, "David S. Miller" <davem@redhat.com> 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 <maxk@qualcomm.com> 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      +--
      --==---/ / _ \/ // /\ \/ /       pcg@goof.com      |e|
      -=====/_/_//_/\_,_/ /_/\_\       XX11-RIPE         --+
    The choice of a GNU generation                       |
                                                         |

Re: 2.4.13-ac5 && vtun not working

[David S. Miller]

   From: <pcg@goof.com ( 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" <davem@redhat.com> 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
davem@redhat.com

Re: 2.4.13-ac5 && vtun not working

[Lehmann]

On Wed, Oct 31, 2001 at 12:30:56AM -0800, "David S. Miller" <davem@redhat.com> 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      +--
      --==---/ / _ \/ // /\ \/ /       pcg@goof.com      |e|
      -=====/_/_//_/\_,_/ /_/\_\       XX11-RIPE         --+
    The choice of a GNU generation                       |
                                                         |

Re: 2.4.13-ac5 && vtun not working

[Maksim Krasnyanskiy]

At 10:43 AM 10/31/01 +0100, pcg@goof.com ( Marc) (A.) (Lehmann ) wrote:
>On Wed, Oct 31, 2001 at 12:30:56AM -0800, "David S. Miller" <davem@redhat.com> 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

Re: 2.4.13-ac5 && vtun not working

[David S. Miller]

   From: Maksim Krasnyanskiy <maxk@qualcomm.com>
   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
davem@redhat.com

Re: 2.4.13-ac5 && vtun not working

[Maksim Krasnyanskiy]

>   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