Xen devel - Mar 2009 - blktap kernel module accessing freed memory?

blktap_release() makes use of the vma stored in blktap_mmap(). This vma,
however, should be invalid by the time control reaches blktap_release(),
as it gets freed from release_vma() (in the context of either exit_mm() or
do_munmap()).

While it seems this was always the case (and always wrong), is seems this
is becoming more of a problem with the mmap_sem locking additions of c/s
719, as that change results in more uses of the possibly no longer initialized
data (we just had a report of info->vma->vm_mm being NULL).

The reason I''m not directly submitting a patch for this is that
I''m uncertain
of the purpose of this extra call to zap_page_range(): While it could be
moved into a to-be-created vm_operations.close actor, it would seem
redundant to do so, since unmap_vmas() already takes care of removing
the mappings for the vma in question.

Regardless of that, the kfree(info->vma->vm_private_data) must be
moved into this to-be-created vm_operations.close actor imo. And in
order to not risk using info->vma past that actor, it might be a good idea
to also clear out info->vma there (namely for fast_flush_area(), which
already checks whether that pointer is non-NULL).

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

>>> "Jan Beulich" <jbeulich@novell.com> 17.03.09 15:22
>>>
>blktap_release() makes use of the vma stored in blktap_mmap(). This vma,
>however, should be invalid by the time control reaches blktap_release(),
>as it gets freed from release_vma() (in the context of either exit_mm() or
>do_munmap()).
>
>While it seems this was always the case (and always wrong), is seems this
>is becoming more of a problem with the mmap_sem locking additions of c/s
>719, as that change results in more uses of the possibly no longer
initialized
>data (we just had a report of info->vma->vm_mm being NULL).
>
>The reason I''m not directly submitting a patch for this is that
I''m uncertain
>of the purpose of this extra call to zap_page_range(): While it could be
>moved into a to-be-created vm_operations.close actor, it would seem
>redundant to do so, since unmap_vmas() already takes care of removing
>the mappings for the vma in question.
>
>Regardless of that, the kfree(info->vma->vm_private_data) must be
>moved into this to-be-created vm_operations.close actor imo. And in
>order to not risk using info->vma past that actor, it might be a good
idea
>to also clear out info->vma there (namely for fast_flush_area(), which
>already checks whether that pointer is non-NULL).
You seem to be the original authors of blktap, so I''m hoping that you
could
comment on this.

Additionally I''m attaching a tentative patch (against 2.6.27.x),
provided
the analysis above is correct.

Thanks a lot,
Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel