Re: [Qemu-devel] 9pfs segfaults on chmod(special)

[Michael Tokarev]

Guys, are we playing with our sand-box toys or what?

Can we apply this maybe to 1.5??  It's just insane that
such a simple bugfixes, with lots of preceeding work to
identify it, and with users suffering, are being simply
ignored for months...

Thanks,

/mjt

28.02.2013 13:12, Aneesh Kumar K.V wrote:
> Michael Tokarev <address@hidden> writes:
> 
>> When guest tries to chmod a block or char device file over 9pfs,
>> the qemu process segfaults.
>>
>> On host:
>>  qemu-system-x86_64 -virtfs 
>> local,path=/dev,security_model=mapped-file,mount_tag=tag
>>
>> On guest:
>>  mount -t 9p -o trans=virtio tag /mnt
>>  chmod 0777 /mnt/tty
> 
> any specific reason why you are trying 9p .u ?
> 
>>
>> Result (for 1.4.0):
>>
>> Program received signal SIGSEGV, Segmentation fault.
>> 0x566095af in v9mode_to_mode (mode=8389101, extension=0xc7584ef8)
>>     at /build/kvm/git/hw/9pfs/virtio-9p.c:662
>> 662          if (extension && extension->data[0] == 'c') {
>> (gdb) p *extension
>> $1 = {size = 0, data = 0x0}
>> (gdb) bt
>> #0  0x566095af in v9mode_to_mode (mode=8389101, extension=0xc7584ef8)
>>     at hw/9pfs/virtio-9p.c:662
>> #1  0x5660f38b in v9fs_wstat (opaque=0xd250945c)
>>     at hw/9pfs/virtio-9p.c:2635
>> (gdb) frame 1
>> #1  0x5660f38b in v9fs_wstat (opaque=0xd250945c)
>>     at hw/9pfs/virtio-9p.c:2635
>> 2635         err = v9fs_co_chmod(pdu, &fidp->path,
>>                             v9mode_to_mode(v9stat.mode,
>>                                            &v9stat.extension));
>> (gdb) p v9stat
>> $2 = {size = 61, type = -1, dev = -1, qid = {type = -1 '\377', version = -1,
>>     path = -1}, mode = 8389101, atime = -1, mtime = -1, length = -1, name = {
>>     size = 0, data = 0x0}, uid = {size = 0, data = 0x0}, gid = {size = 0,
>>     data = 0x0}, muid = {size = 0, data = 0x0}, extension = {size = 0,
>>     data = 0x0}, n_uid = -1, n_gid = -1, n_muid = -1}
>>
>>
>> Corresponding code in v9mode_to_mode():
>>
>>      if (mode & P9_STAT_MODE_DEVICE) {
>>          if (extension && extension->data[0] == 'c') {
>>              ret |= S_IFCHR;
>>          } else {
>>              ret |= S_IFBLK;
>>      }
>>
>> This (static) function (v9mode_to_mode) is called from only one place,
>> namely from v9fs_wstat(), and it always calls it with non-NULL
>> `extension' argument: &v9stat.extension.
>>
>> Maybe the buffer (extension->data) should be passed to it instead of
>> the whole structure (extension)?  Or the check be extended (or,
>> since this function isn't called from any other place, _replaced_) to
>> test for non-NULL ->data too?
>>
> 
> Thanks for the detailed analysis. Something like below ?
> 
> diff --git a/hw/9pfs/virtio-9p.c b/hw/9pfs/virtio-9p.c
> index f526467..073067f 100644
> --- a/hw/9pfs/virtio-9p.c
> +++ b/hw/9pfs/virtio-9p.c
> @@ -659,7 +659,7 @@ static mode_t v9mode_to_mode(uint32_t mode, V9fsString 
> *extension)
>          ret |= S_IFIFO;
>      }
>      if (mode & P9_STAT_MODE_DEVICE) {
> -        if (extension && extension->data[0] == 'c') {
> +        if (extension->size && extension->data[0] == 'c') {
>              ret |= S_IFCHR;
>          } else {
>              ret |= S_IFBLK;
> 
>