[Qemu-devel] [PATCH] Random MAC address option

[Qemu-devel] [PATCH] Random MAC address option

[Pavel Vasilyev]

[-- Attachment #1.1: Type: text/plain, Size: 3818 bytes --]

Index: Makefile.target
===================================================================
--- Makefile.target	(revision 6463)
+++ Makefile.target	(working copy)
@@ -580,7 +580,8 @@
 OBJS+= fdc.o mc146818rtc.o serial.o i8259.o i8254.o pcspk.o pc.o
 OBJS+= cirrus_vga.o apic.o parallel.o acpi.o piix_pci.o
 OBJS+= usb-uhci.o vmmouse.o vmport.o vmware_vga.o hpet.o
-CPPFLAGS += -DHAS_AUDIO -DHAS_AUDIO_CHOICE
+CPPFLAGS += 
+#-DHAS_AUDIO -DHAS_AUDIO_CHOICE
 endif
 ifeq ($(TARGET_BASE_ARCH), ppc)
 CPPFLAGS += -DHAS_AUDIO -DHAS_AUDIO_CHOICE
Index: net.c
===================================================================
--- net.c	(revision 6463)
+++ net.c	(working copy)
@@ -122,6 +122,24 @@
 
 static VLANState *first_vlan;
 
+unsigned long int random_mac_seed(void) {
+
+    unsigned long int seed = 0;
+    int fd;
+
+    fd = open("/dev/urandom", O_RDONLY);
+    if (fd == -1) {
+        perror("open");
+        return 0;
+    }
+    if (read(fd, &seed, sizeof (seed)) < 0) {
+        perror("read");
+        seed = 0;
+    }
+    if (close(fd))
+        perror("close");
+    return seed;
+}
 /***********************************************************/
 /* network device redirectors */
 
@@ -160,7 +178,7 @@
     long int offset;
 
     errno = 0;
-    offset = strtol(p, &last_char, 0);    
+    offset = strtol(p, &last_char, 0);
     if (0 == errno && '\0' == *last_char &&
             offset >= 0 && offset <= 0xFFFFFF) {
         macaddr[3] = (offset & 0xFF0000) >> 16;
@@ -179,7 +197,7 @@
                 p++;
             }
         }
-        return 0;    
+        return 0;
     }
 
     return -1;
@@ -1576,15 +1594,27 @@
             fprintf(stderr, "Too Many NICs\n");
             return -1;
         }
+
         nd = &nd_table[nb_nics];
         macaddr = nd->macaddr;
-        macaddr[0] = 0x52;
-        macaddr[1] = 0x54;
-        macaddr[2] = 0x00;
-        macaddr[3] = 0x12;
-        macaddr[4] = 0x34;
-        macaddr[5] = 0x56 + nb_nics;
 
+        if (strstr(p, "macrandom")) {
+            macaddr[0] = (uint8_t) (random_mac_seed() % 0x100);
+            macaddr[1] = (uint8_t) (random_mac_seed() % 0x100);
+            macaddr[2] = (uint8_t) (random_mac_seed() % 0x100);
+            macaddr[3] = (uint8_t) (random_mac_seed() % 0x100);
+            macaddr[4] = (uint8_t) (random_mac_seed() % 0x100);
+            macaddr[5] = (uint8_t) (random_mac_seed() % 0x100);
+
+        } else {
+            macaddr[0] = 0x52;
+            macaddr[1] = 0x54;
+            macaddr[2] = 0x00;
+            macaddr[3] = 0x12;
+            macaddr[4] = 0x34;
+            macaddr[5] = 0x56 + nb_nics;
+        }
+
         if (get_param_value(buf, sizeof(buf), "macaddr", p)) {
             if (parse_macaddr(macaddr, buf) < 0) {
                 fprintf(stderr, "invalid syntax for ethernet address\n");
@@ -1720,7 +1750,7 @@
     const char *p;
     char *q;
     char device[64];
-
+    
     p = str;
     q = device;
     while (*p != '\0' && *p != ',') {
Index: net.h
===================================================================
--- net.h	(revision 6463)
+++ net.h	(working copy)
@@ -52,6 +52,8 @@
                                const char *default_model);
 void qemu_handler_true(void *opaque);
 
+unsigned long int random_mac_seed(void);
+
 void do_info_network(void);
 int do_set_link(const char *name, const char *up_or_down);
 
Index: qemu-malloc.c
===================================================================
--- qemu-malloc.c	(revision 6463)
+++ qemu-malloc.c	(working copy)
@@ -35,7 +35,7 @@
 
 void *qemu_malloc(size_t size)
 {
-    return malloc(size);
+    return calloc(1, size);
 }
 
 void *qemu_realloc(void *ptr, size_t size)

[-- Attachment #1.2: macrandom.patch --]
[-- Type: text/x-diff, Size: 3818 bytes --]

Index: Makefile.target
===================================================================
--- Makefile.target	(revision 6463)
+++ Makefile.target	(working copy)
@@ -580,7 +580,8 @@
 OBJS+= fdc.o mc146818rtc.o serial.o i8259.o i8254.o pcspk.o pc.o
 OBJS+= cirrus_vga.o apic.o parallel.o acpi.o piix_pci.o
 OBJS+= usb-uhci.o vmmouse.o vmport.o vmware_vga.o hpet.o
-CPPFLAGS += -DHAS_AUDIO -DHAS_AUDIO_CHOICE
+CPPFLAGS += 
+#-DHAS_AUDIO -DHAS_AUDIO_CHOICE
 endif
 ifeq ($(TARGET_BASE_ARCH), ppc)
 CPPFLAGS += -DHAS_AUDIO -DHAS_AUDIO_CHOICE
Index: net.c
===================================================================
--- net.c	(revision 6463)
+++ net.c	(working copy)
@@ -122,6 +122,24 @@
 
 static VLANState *first_vlan;
 
+unsigned long int random_mac_seed(void) {
+
+    unsigned long int seed = 0;
+    int fd;
+
+    fd = open("/dev/urandom", O_RDONLY);
+    if (fd == -1) {
+        perror("open");
+        return 0;
+    }
+    if (read(fd, &seed, sizeof (seed)) < 0) {
+        perror("read");
+        seed = 0;
+    }
+    if (close(fd))
+        perror("close");
+    return seed;
+}
 /***********************************************************/
 /* network device redirectors */
 
@@ -160,7 +178,7 @@
     long int offset;
 
     errno = 0;
-    offset = strtol(p, &last_char, 0);    
+    offset = strtol(p, &last_char, 0);
     if (0 == errno && '\0' == *last_char &&
             offset >= 0 && offset <= 0xFFFFFF) {
         macaddr[3] = (offset & 0xFF0000) >> 16;
@@ -179,7 +197,7 @@
                 p++;
             }
         }
-        return 0;    
+        return 0;
     }
 
     return -1;
@@ -1576,15 +1594,27 @@
             fprintf(stderr, "Too Many NICs\n");
             return -1;
         }
+
         nd = &nd_table[nb_nics];
         macaddr = nd->macaddr;
-        macaddr[0] = 0x52;
-        macaddr[1] = 0x54;
-        macaddr[2] = 0x00;
-        macaddr[3] = 0x12;
-        macaddr[4] = 0x34;
-        macaddr[5] = 0x56 + nb_nics;
 
+        if (strstr(p, "macrandom")) {
+            macaddr[0] = (uint8_t) (random_mac_seed() % 0x100);
+            macaddr[1] = (uint8_t) (random_mac_seed() % 0x100);
+            macaddr[2] = (uint8_t) (random_mac_seed() % 0x100);
+            macaddr[3] = (uint8_t) (random_mac_seed() % 0x100);
+            macaddr[4] = (uint8_t) (random_mac_seed() % 0x100);
+            macaddr[5] = (uint8_t) (random_mac_seed() % 0x100);
+
+        } else {
+            macaddr[0] = 0x52;
+            macaddr[1] = 0x54;
+            macaddr[2] = 0x00;
+            macaddr[3] = 0x12;
+            macaddr[4] = 0x34;
+            macaddr[5] = 0x56 + nb_nics;
+        }
+
         if (get_param_value(buf, sizeof(buf), "macaddr", p)) {
             if (parse_macaddr(macaddr, buf) < 0) {
                 fprintf(stderr, "invalid syntax for ethernet address\n");
@@ -1720,7 +1750,7 @@
     const char *p;
     char *q;
     char device[64];
-
+    
     p = str;
     q = device;
     while (*p != '\0' && *p != ',') {
Index: net.h
===================================================================
--- net.h	(revision 6463)
+++ net.h	(working copy)
@@ -52,6 +52,8 @@
                                const char *default_model);
 void qemu_handler_true(void *opaque);
 
+unsigned long int random_mac_seed(void);
+
 void do_info_network(void);
 int do_set_link(const char *name, const char *up_or_down);
 
Index: qemu-malloc.c
===================================================================
--- qemu-malloc.c	(revision 6463)
+++ qemu-malloc.c	(working copy)
@@ -35,7 +35,7 @@
 
 void *qemu_malloc(size_t size)
 {
-    return malloc(size);
+    return calloc(1, size);
 }
 
 void *qemu_realloc(void *ptr, size_t size)

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 194 bytes --]

[Qemu-devel] [PATCH] Replace strcmp() by strncmp()

[Pavel Vasilyev]

[-- Attachment #1: Type: text/plain, Size: 1506 bytes --]

Possible SIGSEGV 

Index: net.c
===================================================================
--- net.c       (revision 6463)
+++ net.c       (working copy)
@@ -1568,7 +1586,7 @@
     if (get_param_value(buf, sizeof(buf), "name", p)) {
         name = strdup(buf);
     }
-    if (!strcmp(device, "nic")) {
+    if (!strncmp(device, "nic", 3)) {
         NICInfo *nd;
         uint8_t *macaddr;

@@ -1601,13 +1630,13 @@
         vlan->nb_guest_devs++;
         ret = 0;
     } else
-    if (!strcmp(device, "none")) {
+    if (!strncmp(device, "none", 4)) {
         /* does nothing. It is needed to signal that no network cards
            are wanted */
         ret = 0;
     } else
 #ifdef CONFIG_SLIRP
-    if (!strcmp(device, "user")) {
+    if (!strncmp(device, "user", 4)) {
         if (get_param_value(buf, sizeof(buf), "hostname", p)) {
             pstrcpy(slirp_hostname, sizeof(slirp_hostname), buf);
         }
@@ -1622,7 +1651,7 @@
     } else
 #endif
 #ifdef _WIN32
-    if (!strcmp(device, "tap")) {
+    if (!strncmp(device, "tap", 3)) {
         char ifname[64];
         if (get_param_value(ifname, sizeof(ifname), "ifname", p) <= 0) {
             fprintf(stderr, "tap: no interface name\n");
@@ -1633,7 +1662,7 @@
     } else
 #elif defined (_AIX)
 #else
-    if (!strcmp(device, "tap")) {
+    if (!strncmp(device, "tap", 3)) {
         char ifname[64];
         char setup_script[1024], down_script[1024];
         int fd;



[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 194 bytes --]

Re: [Qemu-devel] [PATCH] Random MAC address option

[malc]

On Tue, 27 Jan 2009, Pavel Vasilyev wrote:

> Index: Makefile.target
> ===================================================================
> --- Makefile.target	(revision 6463)
> +++ Makefile.target	(working copy)
> @@ -580,7 +580,8 @@
>  OBJS+= fdc.o mc146818rtc.o serial.o i8259.o i8254.o pcspk.o pc.o
>  OBJS+= cirrus_vga.o apic.o parallel.o acpi.o piix_pci.o
>  OBJS+= usb-uhci.o vmmouse.o vmport.o vmware_vga.o hpet.o
> -CPPFLAGS += -DHAS_AUDIO -DHAS_AUDIO_CHOICE
> +CPPFLAGS += 
> +#-DHAS_AUDIO -DHAS_AUDIO_CHOICE
>  endif
>  ifeq ($(TARGET_BASE_ARCH), ppc)
>  CPPFLAGS += -DHAS_AUDIO -DHAS_AUDIO_CHOICE

I'm not quite sure how disabling audio relates to MAC addresses..

[..snip..]

> Index: qemu-malloc.c
> ===================================================================
> --- qemu-malloc.c	(revision 6463)
> +++ qemu-malloc.c	(working copy)
> @@ -35,7 +35,7 @@
>  
>  void *qemu_malloc(size_t size)
>  {
> -    return malloc(size);
> +    return calloc(1, size);
>  }
>  
>  void *qemu_realloc(void *ptr, size_t size)
> 

Nor this..

-- 
mailto:av1474@comtv.ru

[Qemu-devel] Re: [PATCH] Replace strcmp() by strncmp()

[Jan Kiszka]

Pavel Vasilyev wrote:
> Possible SIGSEGV 

I would suggest studying some strncmp implementation and also thinking
about the case that 'device' is, say, "nics"...

Jan

> 
> Index: net.c
> ===================================================================
> --- net.c       (revision 6463)
> +++ net.c       (working copy)
> @@ -1568,7 +1586,7 @@
>      if (get_param_value(buf, sizeof(buf), "name", p)) {
>          name = strdup(buf);
>      }
> -    if (!strcmp(device, "nic")) {
> +    if (!strncmp(device, "nic", 3)) {
>          NICInfo *nd;
>          uint8_t *macaddr;
> 
> @@ -1601,13 +1630,13 @@
>          vlan->nb_guest_devs++;
>          ret = 0;
>      } else
> -    if (!strcmp(device, "none")) {
> +    if (!strncmp(device, "none", 4)) {
>          /* does nothing. It is needed to signal that no network cards
>             are wanted */
>          ret = 0;
>      } else
>  #ifdef CONFIG_SLIRP
> -    if (!strcmp(device, "user")) {
> +    if (!strncmp(device, "user", 4)) {
>          if (get_param_value(buf, sizeof(buf), "hostname", p)) {
>              pstrcpy(slirp_hostname, sizeof(slirp_hostname), buf);
>          }
> @@ -1622,7 +1651,7 @@
>      } else
>  #endif
>  #ifdef _WIN32
> -    if (!strcmp(device, "tap")) {
> +    if (!strncmp(device, "tap", 3)) {
>          char ifname[64];
>          if (get_param_value(ifname, sizeof(ifname), "ifname", p) <= 0) {
>              fprintf(stderr, "tap: no interface name\n");
> @@ -1633,7 +1662,7 @@
>      } else
>  #elif defined (_AIX)
>  #else
> -    if (!strcmp(device, "tap")) {
> +    if (!strncmp(device, "tap", 3)) {
>          char ifname[64];
>          char setup_script[1024], down_script[1024];
>          int fd;
> 
> 

-- 
Siemens AG, Corporate Technology, CT SE 2
Corporate Competence Center Embedded Linux

Re: [Qemu-devel] [PATCH] Random MAC address option

[Kevin Wolf]

Pavel Vasilyev schrieb:
> Index: Makefile.target
> ===================================================================
> --- Makefile.target	(revision 6463)
> +++ Makefile.target	(working copy)
> @@ -580,7 +580,8 @@
>  OBJS+= fdc.o mc146818rtc.o serial.o i8259.o i8254.o pcspk.o pc.o
>  OBJS+= cirrus_vga.o apic.o parallel.o acpi.o piix_pci.o
>  OBJS+= usb-uhci.o vmmouse.o vmport.o vmware_vga.o hpet.o
> -CPPFLAGS += -DHAS_AUDIO -DHAS_AUDIO_CHOICE
> +CPPFLAGS += 
> +#-DHAS_AUDIO -DHAS_AUDIO_CHOICE
>  endif
>  ifeq ($(TARGET_BASE_ARCH), ppc)
>  CPPFLAGS += -DHAS_AUDIO -DHAS_AUDIO_CHOICE
> Index: net.c
> ===================================================================
> --- net.c	(revision 6463)
> +++ net.c	(working copy)
> @@ -122,6 +122,24 @@
>  
>  static VLANState *first_vlan;
>  
> +unsigned long int random_mac_seed(void) {
> +
> +    unsigned long int seed = 0;
> +    int fd;
> +
> +    fd = open("/dev/urandom", O_RDONLY);

How is that supposed to work on Windows? And what's wrong with rand()?

> +    if (fd == -1) {
> +        perror("open");
> +        return 0;
> +    }
> +    if (read(fd, &seed, sizeof (seed)) < 0) {
> +        perror("read");
> +        seed = 0;
> +    }
> +    if (close(fd))
> +        perror("close");
> +    return seed;
> +}
>  /***********************************************************/
>  /* network device redirectors */
>  
> @@ -160,7 +178,7 @@
>      long int offset;
>  
>      errno = 0;
> -    offset = strtol(p, &last_char, 0);    
> +    offset = strtol(p, &last_char, 0);
>      if (0 == errno && '\0' == *last_char &&
>              offset >= 0 && offset <= 0xFFFFFF) {
>          macaddr[3] = (offset & 0xFF0000) >> 16;
> @@ -179,7 +197,7 @@
>                  p++;
>              }
>          }
> -        return 0;    
> +        return 0;
>      }
>  
>      return -1;

I don't think these whitespace changes should be there.

Kevin

Re: [Qemu-devel] [PATCH] Random MAC address option

[Riku Voipio]

On Tue, Jan 27, 2009 at 03:13:18AM +0300, Pavel Vasilyev wrote:
> +        if (strstr(p, "macrandom")) {

I take this means -net macaddr=macrandom will give the random
macaddr after this patch? please document this magic in
qemu-doc.texi and --help.

Re: [Qemu-devel] [PATCH] Random MAC address option

[Daniel P. Berrange]

On Tue, Jan 27, 2009 at 12:15:41PM +0200, Riku Voipio wrote:
> On Tue, Jan 27, 2009 at 03:13:18AM +0300, Pavel Vasilyev wrote:
> > +        if (strstr(p, "macrandom")) {
> 
> I take this means -net macaddr=macrandom will give the random
> macaddr after this patch? please document this magic in
> qemu-doc.texi and --help.

Generating a random MAC address everytime you boot a guest seems like
a very odd thing todo. MAC addresses should be  randomly generated
once, and then fixed for the duration of existance of that guest,
across reboots.If you generate a random MAC on every boot, the OS will
think its NIC has been removed, and a new one added on every boot.

Daniel
-- 
|: Red Hat, Engineering, London   -o-   http://people.redhat.com/berrange/ :|
|: http://libvirt.org  -o-  http://virt-manager.org  -o-  http://ovirt.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505  -o-  F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|

Re: [Qemu-devel] [PATCH] Random MAC address option

[Ian Jackson]

Daniel P. Berrange writes ("Re: [Qemu-devel] [PATCH] Random MAC address option"):
> On Tue, Jan 27, 2009 at 12:15:41PM +0200, Riku Voipio wrote:
> > I take this means -net macaddr=macrandom will give the random
> > macaddr after this patch? please document this magic in
> > qemu-doc.texi and --help.

Surely `macaddr=macrandom' is tautologous ?  What's wrong with
`macaddr=random' ?

> Generating a random MAC address everytime you boot a guest seems like
> a very odd thing todo. MAC addresses should be  randomly generated
> once, and then fixed for the duration of existance of that guest,
> across reboots.If you generate a random MAC on every boot, the OS will
> think its NIC has been removed, and a new one added on every boot.

Whether or not this is a problem depends on your operating system.
Indeed having a new MAC address each time can have privacy advantages
in certain IPv6 configurations!

If you want the MAC address to be persistent, where should qemu store
the MAC address ?

Ian.

Re: [Qemu-devel] [PATCH] Random MAC address option

[Paul Brook]

> +        if (strstr(p, "macrandom")) {

I'm not convinced this is a useful feature.

We already allow the user to specify a mac address, so this feature can be 
trivially implemented (and better supported) by a management app or simple 
shell script.

In addition to the issues other people have raised, there's no validation that 
the random value is actually a valid mac address, and no conflict resolution 
to avoid duplicate addresses when multiple NICs are present.

Paul

Re: [Qemu-devel] [PATCH] Random MAC address option

[Anthony Liguori]

Paul Brook wrote:
>> +        if (strstr(p, "macrandom")) {
>>     
>
> I'm not convinced this is a useful feature.
>
> We already allow the user to specify a mac address, so this feature can be 
> trivially implemented (and better supported) by a management app or simple 
> shell script.
>
> In addition to the issues other people have raised, there's no validation that 
> the random value is actually a valid mac address, and no conflict resolution 
> to avoid duplicate addresses when multiple NICs are present.
>   

I agree.  It gives a false sense of security.

Regards,

Anthony Liguori

> Paul
>
>
>   

Re: [Qemu-devel] Re: [PATCH] Replace strcmp() by strncmp()

[Pavel Vasilyev]

[-- Attachment #1: Type: text/plain, Size: 256 bytes --]

On Tuesday 27 January 2009 12:28:55 Jan Kiszka wrote:
> Pavel Vasilyev wrote:
> > Possible SIGSEGV
>
> I would suggest studying some strncmp implementation and also thinking
> about the case that 'device' is, say, "nics"...
>
> Jan
>

strncasecmp(); ???



[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 194 bytes --]

Re: [Qemu-devel] [PATCH] Random MAC address option

[Pavel Vasilyev]

[-- Attachment #1: Type: text/plain, Size: 357 bytes --]

On Tuesday 27 January 2009 13:15:41 Riku Voipio wrote:
> On Tue, Jan 27, 2009 at 03:13:18AM +0300, Pavel Vasilyev wrote:
> > +        if (strstr(p, "macrandom")) {
>
> I take this means -net macaddr=macrandom will give the random
> macaddr after this patch? 

-net nic,macrandom 

> please document this magic in 
> qemu-doc.texi and --help.



[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 194 bytes --]

Re: [Qemu-devel] Re: [PATCH] Replace strcmp() by strncmp()

[Pavel Vasilyev]

[-- Attachment #1: Type: text/plain, Size: 466 bytes --]

On Tuesday 27 January 2009 12:28:55 Jan Kiszka wrote:
> Pavel Vasilyev wrote:
> > Possible SIGSEGV
>
> I would suggest studying some strncmp implementation and also thinking
> about the case that 'device' is, say, "nics"...

1. "nics" is other option 
2. strncmp() conforming with next standarts:  SVID 3, POSIX, BSD 4.3, ISO 9899   
3. For case insensitivity - use strncasecmp(), For OSes where there is no this   
function, it is possible to use macro.

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 194 bytes --]