[Qemu-devel] [PATCH] monitor: properly handle invalid fd/vhostfd from command line

[Qemu-devel] [PATCH] monitor: properly handle invalid fd/vhostfd from command line

[Jason Wang]

monitor_get_fd() may also be used to parse fd or vhostfd from command line, so
we need to check whether the pointer of mon is NULL to avoid segmentation fault
when user pass invalid name of fd or vhostfd.

Signed-off-by: Jason Wang <jasowang@redhat.com>
---
 monitor.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/monitor.c b/monitor.c
index e602480..5bb4ff0 100644
--- a/monitor.c
+++ b/monitor.c
@@ -2345,6 +2345,10 @@ int monitor_get_fd(Monitor *mon, const char *fdname)
 {
     mon_fd_t *monfd;
 
+    if (mon == NULL) {
+        return -1;
+    }
+
     QLIST_FOREACH(monfd, &mon->fds, next) {
         int fd;
 

[Qemu-devel] Re: [PATCH] monitor: properly handle invalid fd/vhostfd from command line

[Michael S. Tsirkin]

On Mon, Sep 27, 2010 at 03:52:44PM +0800, Jason Wang wrote:
> monitor_get_fd() may also be used to parse fd or vhostfd from command line, so
> we need to check whether the pointer of mon is NULL to avoid segmentation fault
> when user pass invalid name of fd or vhostfd.
> 
> Signed-off-by: Jason Wang <jasowang@redhat.com>

Acked-by: Michael S. Tsirkin <mst@redhat.com>

> ---
>  monitor.c |    4 ++++
>  1 files changed, 4 insertions(+), 0 deletions(-)
> 
> diff --git a/monitor.c b/monitor.c
> index e602480..5bb4ff0 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -2345,6 +2345,10 @@ int monitor_get_fd(Monitor *mon, const char *fdname)
>  {
>      mon_fd_t *monfd;
>  
> +    if (mon == NULL) {
> +        return -1;
> +    }
> +
>      QLIST_FOREACH(monfd, &mon->fds, next) {
>          int fd;
>  
> 

Re: [Qemu-devel] [PATCH] monitor: properly handle invalid fd/vhostfd from command line

[Luiz Capitulino]

On Mon, 27 Sep 2010 15:52:44 +0800
Jason Wang <jasowang@redhat.com> wrote:

> monitor_get_fd() may also be used to parse fd or vhostfd from command line, so
> we need to check whether the pointer of mon is NULL to avoid segmentation fault
> when user pass invalid name of fd or vhostfd.

Invalid fdname is handled just fine, I have the impression this patch fixes
something else.

Could you elaborate on the real problem here and/or show to reproduce?

> Signed-off-by: Jason Wang <jasowang@redhat.com>
> ---
>  monitor.c |    4 ++++
>  1 files changed, 4 insertions(+), 0 deletions(-)
> 
> diff --git a/monitor.c b/monitor.c
> index e602480..5bb4ff0 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -2345,6 +2345,10 @@ int monitor_get_fd(Monitor *mon, const char *fdname)
>  {
>      mon_fd_t *monfd;
>  
> +    if (mon == NULL) {
> +        return -1;
> +    }
> +
>      QLIST_FOREACH(monfd, &mon->fds, next) {
>          int fd;
>  
> 
> 

Re: [Qemu-devel] [PATCH] monitor: properly handle invalid fd/vhostfd from command line

[Michael S. Tsirkin]

On Tue, Sep 28, 2010 at 11:53:43AM -0300, Luiz Capitulino wrote:
> On Mon, 27 Sep 2010 15:52:44 +0800
> Jason Wang <jasowang@redhat.com> wrote:
> 
> > monitor_get_fd() may also be used to parse fd or vhostfd from command line, so
> > we need to check whether the pointer of mon is NULL to avoid segmentation fault
> > when user pass invalid name of fd or vhostfd.
> 
> Invalid fdname is handled just fine, I have the impression this patch fixes
> something else.
> 
> Could you elaborate on the real problem here and/or show to reproduce?

Try pasing fd= (no value) as a parameter, and see what happens.


> > Signed-off-by: Jason Wang <jasowang@redhat.com>
> > ---
> >  monitor.c |    4 ++++
> >  1 files changed, 4 insertions(+), 0 deletions(-)
> > 
> > diff --git a/monitor.c b/monitor.c
> > index e602480..5bb4ff0 100644
> > --- a/monitor.c
> > +++ b/monitor.c
> > @@ -2345,6 +2345,10 @@ int monitor_get_fd(Monitor *mon, const char *fdname)
> >  {
> >      mon_fd_t *monfd;
> >  
> > +    if (mon == NULL) {
> > +        return -1;
> > +    }
> > +
> >      QLIST_FOREACH(monfd, &mon->fds, next) {
> >          int fd;
> >  
> > 
> > 

Re: [Qemu-devel] [PATCH] monitor: properly handle invalid fd/vhostfd from command line

[Luiz Capitulino]

On Tue, 28 Sep 2010 16:57:44 +0200
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Tue, Sep 28, 2010 at 11:53:43AM -0300, Luiz Capitulino wrote:
> > On Mon, 27 Sep 2010 15:52:44 +0800
> > Jason Wang <jasowang@redhat.com> wrote:
> > 
> > > monitor_get_fd() may also be used to parse fd or vhostfd from command line, so
> > > we need to check whether the pointer of mon is NULL to avoid segmentation fault
> > > when user pass invalid name of fd or vhostfd.
> > 
> > Invalid fdname is handled just fine, I have the impression this patch fixes
> > something else.
> > 
> > Could you elaborate on the real problem here and/or show to reproduce?
> 
> Try pasing fd= (no value) as a parameter, and see what happens.

Sorry for the delay on this one.

If I'm reading this code correctly, we have two kinds of fd passing being
handled by net_handle_fd_param(): fds passed via SCM_RIGHTS (handled by
the monitor) and -net fds, passed via the command-line.

In this case, we don't want the fd passed via SCM_RIGHTS, so
net_handle_fd_param() has to be fixed to check for !mon and also check
strtol()'s return:

diff --git a/net.c b/net.c
index 3d0fde7..6aaa653 100644
--- a/net.c
+++ b/net.c
@@ -739,9 +739,9 @@ int qemu_find_nic_model(NICInfo *nd, const char * const *models,
 
 int net_handle_fd_param(Monitor *mon, const char *param)
 {
-    if (!qemu_isdigit(param[0])) {
-        int fd;
+    int fd;
 
+    if (!qemu_isdigit(param[0]) && mon) {
         fd = monitor_get_fd(mon, param);
         if (fd == -1) {
             error_report("No file descriptor named %s found", param);
@@ -750,7 +750,13 @@ int net_handle_fd_param(Monitor *mon, const char *param)
 
         return fd;
     } else {
-        return strtol(param, NULL, 0);
+        char *endptr = NULL;
+
+        fd = strtol(param, &endptr, 10);
+        if (*endptr || (fd == 0 && param == endptr)) {
+            return -1;
+        }
+        return fd;
     }
 }

There's more: qemu doesn't exit when an invalid fd is passed:

"""
~/stuff/virt/ ./qemu-qmp -hda disks/test.img -enable-kvm -m 1G -snapshot -net tap,fd=40
qemu-qmp: -net tap,fd=40: TUNGETIFF ioctl() failed: Bad file descriptor
TUNSETOFFLOAD ioctl() failed: Bad file descriptor
Warning: vlan 0 with no nics
"""

And QEMU is up and running...


>
> > > Signed-off-by: Jason Wang <jasowang@redhat.com>
> > > ---
> > >  monitor.c |    4 ++++
> > >  1 files changed, 4 insertions(+), 0 deletions(-)
> > > 
> > > diff --git a/monitor.c b/monitor.c
> > > index e602480..5bb4ff0 100644
> > > --- a/monitor.c
> > > +++ b/monitor.c
> > > @@ -2345,6 +2345,10 @@ int monitor_get_fd(Monitor *mon, const char *fdname)
> > >  {
> > >      mon_fd_t *monfd;
> > >  
> > > +    if (mon == NULL) {
> > > +        return -1;
> > > +    }
> > > +
> > >      QLIST_FOREACH(monfd, &mon->fds, next) {
> > >          int fd;
> > >  
> > > 
> > > 
> 

Re: [Qemu-devel] [PATCH] monitor: properly handle invalid fd/vhostfd from command line

[jason wang]

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

On 10/09/2010 01:40 AM, Luiz Capitulino wrote:
> On Tue, 28 Sep 2010 16:57:44 +0200
> "Michael S. Tsirkin"<mst@redhat.com>  wrote:
>
>    
>> On Tue, Sep 28, 2010 at 11:53:43AM -0300, Luiz Capitulino wrote:
>>      
>>> On Mon, 27 Sep 2010 15:52:44 +0800
>>> Jason Wang<jasowang@redhat.com>  wrote:
>>>
>>>        
>>>> monitor_get_fd() may also be used to parse fd or vhostfd from command line, so
>>>> we need to check whether the pointer of mon is NULL to avoid segmentation fault
>>>> when user pass invalid name of fd or vhostfd.
>>>>          
>>> Invalid fdname is handled just fine, I have the impression this patch fixes
>>> something else.
>>>
>>> Could you elaborate on the real problem here and/or show to reproduce?
>>>        
>> Try pasing fd= (no value) as a parameter, and see what happens.
>>      
> Sorry for the delay on this one.
>
> If I'm reading this code correctly, we have two kinds of fd passing being
> handled by net_handle_fd_param(): fds passed via SCM_RIGHTS (handled by
> the monitor) and -net fds, passed via the command-line.
>
> In this case, we don't want the fd passed via SCM_RIGHTS, so
> net_handle_fd_param() has to be fixed to check for !mon and also check
> strtol()'s return:
>    
Yes, it's better to check strtol()'s return which was missed in the 
original patch.
> diff --git a/net.c b/net.c
> index 3d0fde7..6aaa653 100644
> --- a/net.c
> +++ b/net.c
> @@ -739,9 +739,9 @@ int qemu_find_nic_model(NICInfo *nd, const char * const *models,
>
>   int net_handle_fd_param(Monitor *mon, const char *param)
>   {
> -    if (!qemu_isdigit(param[0])) {
> -        int fd;
> +    int fd;
>
> +    if (!qemu_isdigit(param[0])&&  mon) {
>           fd = monitor_get_fd(mon, param);
>           if (fd == -1) {
>               error_report("No file descriptor named %s found", param);
> @@ -750,7 +750,13 @@ int net_handle_fd_param(Monitor *mon, const char *param)
>
>           return fd;
>       } else {
> -        return strtol(param, NULL, 0);
> +        char *endptr = NULL;
> +
> +        fd = strtol(param,&endptr, 10);
> +        if (*endptr || (fd == 0&&  param == endptr)) {
> +            return -1;
> +        }
> +        return fd;
>       }
>   }
>    
This looks good for me.
> There's more: qemu doesn't exit when an invalid fd is passed:
>
> """
> ~/stuff/virt/ ./qemu-qmp -hda disks/test.img -enable-kvm -m 1G -snapshot -net tap,fd=40
> qemu-qmp: -net tap,fd=40: TUNGETIFF ioctl() failed: Bad file descriptor
> TUNSETOFFLOAD ioctl() failed: Bad file descriptor
> Warning: vlan 0 with no nics
> """
>
> And QEMU is up and running...//
>    
Anthony have proposed a patch for this.
//
>
>    
>>      
>>>> Signed-off-by: Jason Wang<jasowang@redhat.com>
>>>> ---
>>>>   monitor.c |    4 ++++
>>>>   1 files changed, 4 insertions(+), 0 deletions(-)
>>>>
>>>> diff --git a/monitor.c b/monitor.c
>>>> index e602480..5bb4ff0 100644
>>>> --- a/monitor.c
>>>> +++ b/monitor.c
>>>> @@ -2345,6 +2345,10 @@ int monitor_get_fd(Monitor *mon, const char *fdname)
>>>>   {
>>>>       mon_fd_t *monfd;
>>>>
>>>> +    if (mon == NULL) {
>>>> +        return -1;
>>>> +    }
>>>> +
>>>>       QLIST_FOREACH(monfd,&mon->fds, next) {
>>>>           int fd;
>>>>
>>>>
>>>>
>>>>          
>>      
>
>    


[-- Attachment #2: Type: text/html, Size: 4389 bytes --]

Re: [Qemu-devel] [PATCH] monitor: properly handle invalid fd/vhostfd from command line

[Luiz Capitulino]

On Tue, 12 Oct 2010 14:32:25 +0800
jason wang <jasowang@redhat.com> wrote:

> On 10/09/2010 01:40 AM, Luiz Capitulino wrote:
> > On Tue, 28 Sep 2010 16:57:44 +0200
> > "Michael S. Tsirkin"<mst@redhat.com>  wrote:
> >
> >    
> >> On Tue, Sep 28, 2010 at 11:53:43AM -0300, Luiz Capitulino wrote:
> >>      
> >>> On Mon, 27 Sep 2010 15:52:44 +0800
> >>> Jason Wang<jasowang@redhat.com>  wrote:
> >>>
> >>>        
> >>>> monitor_get_fd() may also be used to parse fd or vhostfd from command line, so
> >>>> we need to check whether the pointer of mon is NULL to avoid segmentation fault
> >>>> when user pass invalid name of fd or vhostfd.
> >>>>          
> >>> Invalid fdname is handled just fine, I have the impression this patch fixes
> >>> something else.
> >>>
> >>> Could you elaborate on the real problem here and/or show to reproduce?
> >>>        
> >> Try pasing fd= (no value) as a parameter, and see what happens.
> >>      
> > Sorry for the delay on this one.
> >
> > If I'm reading this code correctly, we have two kinds of fd passing being
> > handled by net_handle_fd_param(): fds passed via SCM_RIGHTS (handled by
> > the monitor) and -net fds, passed via the command-line.
> >
> > In this case, we don't want the fd passed via SCM_RIGHTS, so
> > net_handle_fd_param() has to be fixed to check for !mon and also check
> > strtol()'s return:
> >    
> Yes, it's better to check strtol()'s return which was missed in the 
> original patch.
> > diff --git a/net.c b/net.c
> > index 3d0fde7..6aaa653 100644
> > --- a/net.c
> > +++ b/net.c
> > @@ -739,9 +739,9 @@ int qemu_find_nic_model(NICInfo *nd, const char * const *models,
> >
> >   int net_handle_fd_param(Monitor *mon, const char *param)
> >   {
> > -    if (!qemu_isdigit(param[0])) {
> > -        int fd;
> > +    int fd;
> >
> > +    if (!qemu_isdigit(param[0])&&  mon) {
> >           fd = monitor_get_fd(mon, param);
> >           if (fd == -1) {
> >               error_report("No file descriptor named %s found", param);
> > @@ -750,7 +750,13 @@ int net_handle_fd_param(Monitor *mon, const char *param)
> >
> >           return fd;
> >       } else {
> > -        return strtol(param, NULL, 0);
> > +        char *endptr = NULL;
> > +
> > +        fd = strtol(param,&endptr, 10);
> > +        if (*endptr || (fd == 0&&  param == endptr)) {
> > +            return -1;
> > +        }
> > +        return fd;
> >       }
> >   }
> >    
> This looks good for me.

Would you mind taking care of this one? I mean, I didn't even test this much :-)

So, you could test it more and submit it again as a proper patch.

Thanks.

> > There's more: qemu doesn't exit when an invalid fd is passed:
> >
> > """
> > ~/stuff/virt/ ./qemu-qmp -hda disks/test.img -enable-kvm -m 1G -snapshot -net tap,fd=40
> > qemu-qmp: -net tap,fd=40: TUNGETIFF ioctl() failed: Bad file descriptor
> > TUNSETOFFLOAD ioctl() failed: Bad file descriptor
> > Warning: vlan 0 with no nics
> > """
> >
> > And QEMU is up and running...//
> >    
> Anthony have proposed a patch for this.
> //
> >
> >    
> >>      
> >>>> Signed-off-by: Jason Wang<jasowang@redhat.com>
> >>>> ---
> >>>>   monitor.c |    4 ++++
> >>>>   1 files changed, 4 insertions(+), 0 deletions(-)
> >>>>
> >>>> diff --git a/monitor.c b/monitor.c
> >>>> index e602480..5bb4ff0 100644
> >>>> --- a/monitor.c
> >>>> +++ b/monitor.c
> >>>> @@ -2345,6 +2345,10 @@ int monitor_get_fd(Monitor *mon, const char *fdname)
> >>>>   {
> >>>>       mon_fd_t *monfd;
> >>>>
> >>>> +    if (mon == NULL) {
> >>>> +        return -1;
> >>>> +    }
> >>>> +
> >>>>       QLIST_FOREACH(monfd,&mon->fds, next) {
> >>>>           int fd;
> >>>>
> >>>>
> >>>>
> >>>>          
> >>      
> >
> >    
> 

Re: [Qemu-devel] [PATCH] monitor: properly handle invalid fd/vhostfd from command line

[Jason Wang]

----- "Luiz Capitulino" <lcapitulino@redhat.com> wrote:

> On Tue, 12 Oct 2010 14:32:25 +0800
> jason wang <jasowang@redhat.com> wrote:
> 
> > On 10/09/2010 01:40 AM, Luiz Capitulino wrote:
> > > On Tue, 28 Sep 2010 16:57:44 +0200
> > > "Michael S. Tsirkin"<mst@redhat.com>  wrote:
> > >
> > >    
> > >> On Tue, Sep 28, 2010 at 11:53:43AM -0300, Luiz Capitulino wrote:
> > >>      
> > >>> On Mon, 27 Sep 2010 15:52:44 +0800
> > >>> Jason Wang<jasowang@redhat.com>  wrote:
> > >>>
> > >>>        
> > >>>> monitor_get_fd() may also be used to parse fd or vhostfd from
> command line, so
> > >>>> we need to check whether the pointer of mon is NULL to avoid
> segmentation fault
> > >>>> when user pass invalid name of fd or vhostfd.
> > >>>>          
> > >>> Invalid fdname is handled just fine, I have the impression this
> patch fixes
> > >>> something else.
> > >>>
> > >>> Could you elaborate on the real problem here and/or show to
> reproduce?
> > >>>        
> > >> Try pasing fd= (no value) as a parameter, and see what happens.
> > >>      
> > > Sorry for the delay on this one.
> > >
> > > If I'm reading this code correctly, we have two kinds of fd
> passing being
> > > handled by net_handle_fd_param(): fds passed via SCM_RIGHTS
> (handled by
> > > the monitor) and -net fds, passed via the command-line.
> > >
> > > In this case, we don't want the fd passed via SCM_RIGHTS, so
> > > net_handle_fd_param() has to be fixed to check for !mon and also
> check
> > > strtol()'s return:
> > >    
> > Yes, it's better to check strtol()'s return which was missed in the
> 
> > original patch.
> > > diff --git a/net.c b/net.c
> > > index 3d0fde7..6aaa653 100644
> > > --- a/net.c
> > > +++ b/net.c
> > > @@ -739,9 +739,9 @@ int qemu_find_nic_model(NICInfo *nd, const
> char * const *models,
> > >
> > >   int net_handle_fd_param(Monitor *mon, const char *param)
> > >   {
> > > -    if (!qemu_isdigit(param[0])) {
> > > -        int fd;
> > > +    int fd;
> > >
> > > +    if (!qemu_isdigit(param[0])&&  mon) {
> > >           fd = monitor_get_fd(mon, param);
> > >           if (fd == -1) {
> > >               error_report("No file descriptor named %s found",
> param);
> > > @@ -750,7 +750,13 @@ int net_handle_fd_param(Monitor *mon, const
> char *param)
> > >
> > >           return fd;
> > >       } else {
> > > -        return strtol(param, NULL, 0);
> > > +        char *endptr = NULL;
> > > +
> > > +        fd = strtol(param,&endptr, 10);
> > > +        if (*endptr || (fd == 0&&  param == endptr)) {
> > > +            return -1;
> > > +        }
> > > +        return fd;
> > >       }
> > >   }
> > >    
> > This looks good for me.
> 
> Would you mind taking care of this one? I mean, I didn't even test
> this much :-)
> 
> So, you could test it more and submit it again as a proper patch.
> 
> Thanks.

Sure, would post this later.

> 
> > > There's more: qemu doesn't exit when an invalid fd is passed:
> > >
> > > """
> > > ~/stuff/virt/ ./qemu-qmp -hda disks/test.img -enable-kvm -m 1G
> -snapshot -net tap,fd=40
> > > qemu-qmp: -net tap,fd=40: TUNGETIFF ioctl() failed: Bad file
> descriptor
> > > TUNSETOFFLOAD ioctl() failed: Bad file descriptor
> > > Warning: vlan 0 with no nics
> > > """
> > >
> > > And QEMU is up and running...//
> > >    
> > Anthony have proposed a patch for this.
> > //
> > >
> > >    
> > >>      
> > >>>> Signed-off-by: Jason Wang<jasowang@redhat.com>
> > >>>> ---
> > >>>>   monitor.c |    4 ++++
> > >>>>   1 files changed, 4 insertions(+), 0 deletions(-)
> > >>>>
> > >>>> diff --git a/monitor.c b/monitor.c
> > >>>> index e602480..5bb4ff0 100644
> > >>>> --- a/monitor.c
> > >>>> +++ b/monitor.c
> > >>>> @@ -2345,6 +2345,10 @@ int monitor_get_fd(Monitor *mon, const
> char *fdname)
> > >>>>   {
> > >>>>       mon_fd_t *monfd;
> > >>>>
> > >>>> +    if (mon == NULL) {
> > >>>> +        return -1;
> > >>>> +    }
> > >>>> +
> > >>>>       QLIST_FOREACH(monfd,&mon->fds, next) {
> > >>>>           int fd;
> > >>>>
> > >>>>
> > >>>>
> > >>>>          
> > >>      
> > >
> > >    
> >