From: Prasad J Pandit <pjp@fedoraproject.org>

The interface name in Linux interface request struct 'ifreq'
OR in qemu-bridge-helper is defined to be of size IFNAMSIZ(=16),
including the terminating null('\0') byte.

QEMU tap device, while invoking qemu-bridge-helper, supplies bridge
name of 16 characters, restrict it to IFNAMESIZ-1 to accommodate
terminating null('\0') byte.

Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org>
---
 net/tap.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/tap.c b/net/tap.c
index e8aadd8d4b..ca8536624c 100644
--- a/net/tap.c
+++ b/net/tap.c
@@ -499,7 +499,7 @@ static int net_bridge_run_helper(const char *helper, const char *bridge,
     if (pid == 0) {
         int open_max = sysconf(_SC_OPEN_MAX), i;
         char fd_buf[6+10];
-        char br_buf[6+IFNAMSIZ] = {0};
+        char br_buf[5+IFNAMSIZ] = {0};
         char helper_cmd[PATH_MAX + sizeof(fd_buf) + sizeof(br_buf) + 15];
 
         for (i = 3; i < open_max; i++) {
-- 
2.21.0

On Mon, Jul 01, 2019 at 02:39:04PM +0530, P J P wrote:
> From: Prasad J Pandit <pjp@fedoraproject.org>
> 
> The interface name in Linux interface request struct 'ifreq'
> OR in qemu-bridge-helper is defined to be of size IFNAMSIZ(=16),
> including the terminating null('\0') byte.
> 
> QEMU tap device, while invoking qemu-bridge-helper, supplies bridge
> name of 16 characters, restrict it to IFNAMESIZ-1 to accommodate
> terminating null('\0') byte.
> 
> Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org>
> ---
>  net/tap.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/net/tap.c b/net/tap.c
> index e8aadd8d4b..ca8536624c 100644
> --- a/net/tap.c
> +++ b/net/tap.c
> @@ -499,7 +499,7 @@ static int net_bridge_run_helper(const char *helper, const char *bridge,
>      if (pid == 0) {
>          int open_max = sysconf(_SC_OPEN_MAX), i;
>          char fd_buf[6+10];
> -        char br_buf[6+IFNAMSIZ] = {0};
> +        char br_buf[5+IFNAMSIZ] = {0};
>          char helper_cmd[PATH_MAX + sizeof(fd_buf) + sizeof(br_buf) + 15];
>  
>          for (i = 3; i < open_max; i++) {

Playing games with multiple "perfectly" sized static buffers & snprintf
is madness. How about re-writing this method so that it just uses
g_strdup_printf() to dynamically format the helper_cmd string.

Alternatively we could get rid of the use of shell and directly exec
the helper program. This would let us just pass argv[] and avoid the
printf'ing entirely.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

+-- On Mon, 1 Jul 2019, Daniel P. Berrangé wrote --+
| Playing games with multiple "perfectly" sized static buffers & snprintf is 
| madness. How about re-writing this method so that it just uses 
| g_strdup_printf() to dynamically format the helper_cmd string.
| 
| Alternatively we could get rid of the use of shell and directly exec the 
| helper program. This would let us just pass argv[] and avoid the printf'ing 
| entirely.

Okay, makes sense; I'll prepare patch v3.

Thank you.
--
Prasad J Pandit / Red Hat Product Security Team
47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F