Patchwork monitor: properly handle invalid fd/vhostfd from command line

login
register
mail settings
Submitter Luiz Capitulino
Date Oct. 8, 2010, 5:40 p.m.
Message ID <20101008144002.245d55e1@doriath>
Download mbox | patch
Permalink /patch/67253/
State New
Headers show

Comments

Luiz Capitulino - Oct. 8, 2010, 5:40 p.m.
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:

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;
> > >  
> > > 
> > > 
>
Jason Wang - Oct. 12, 2010, 6:32 a.m.
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;
>>>>
>>>>
>>>>
>>>>          
>>      
>
>
Luiz Capitulino - Oct. 13, 2010, 1:35 p.m.
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;
> >>>>
> >>>>
> >>>>
> >>>>          
> >>      
> >
> >    
>
Jason Wang - Oct. 14, 2010, 2:21 a.m.
----- "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;
> > >>>>
> > >>>>
> > >>>>
> > >>>>          
> > >>      
> > >
> > >    
> >

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;
     }
 }