[drm-nouveau-mmu] question about potential NULL pointer dereference

[Gustavo A. R. Silva]

Hi all,

While doing some static analysis I ran into the following piece of  
code at drivers/gpu/drm/nouveau/nvkm/subdev/mmu/vmm.c:957:


 957#define node(root, dir) ((root)->head.dir == >list) ? NULL :  
 \

 958list_entry((root)->head.dir, struct nvkm_vma, head)
 959
 960void
 961nvkm_vmm_unmap_region(struct nvkm_vmm *vmm, struct nvkm_vma *vma)
 962{
 963struct nvkm_vma *next;
 964
 965nvkm_memory_tags_put(vma->memory,  
vmm->mmu->subdev.device, >tags);

 966nvkm_memory_unref(>memory);
 967
 968if (vma->part) {
 969struct nvkm_vma *prev = node(vma, prev);
 970if (!prev->memory) {
 971prev->size += vma->size;
 972rb_erase(>tree, >root);
 973list_del(>head);
 974kfree(vma);
 975vma = prev;
 976}
 977}
 978
 979next = node(vma, next);
 980if (next && next->part) {
 981if (!next->memory) {
 982vma->size += next->size;
 983rb_erase(>tree, >root);
 984list_del(>head);
 985kfree(next);
 986}
 987}
 988}

The issue here is that in case _node_ returns NULL, _prev_ is not  
being null checked, hence there is a potential null pointer  
dereference at line 970.


Notice that _next_ is being null checked at line 980, so I wonder if  
_prev_ should be checked the same as _next_.


The fact that both _next_ and next->part are null checked, makes me  
wonder if in case _prev_ actually needs to be checked, there is  
another pointer contained into _prev_ to be validated as well? I'm  
sorry, this is not clear to me at this moment.


I appreciate your feedback
Thank you
--
Gustavo






___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [drm-nouveau-mmu] question about potential NULL pointer dereference

[Gustavo A. R. Silva]

Quoting Ben Skeggs :


On Wed, Feb 14, 2018 at 1:40 AM, Gustavo A. R. Silva
 wrote:


Hi all,

While doing some static analysis I ran into the following piece of code at
drivers/gpu/drm/nouveau/nvkm/subdev/mmu/vmm.c:957:

 957#define node(root, dir) ((root)->head.dir == >list) ? NULL :
\
 958list_entry((root)->head.dir, struct nvkm_vma, head)
 959
 960void
 961nvkm_vmm_unmap_region(struct nvkm_vmm *vmm, struct nvkm_vma *vma)
 962{
 963struct nvkm_vma *next;
 964
 965nvkm_memory_tags_put(vma->memory, vmm->mmu->subdev.device,
>tags);
 966nvkm_memory_unref(>memory);
 967
 968if (vma->part) {
 969struct nvkm_vma *prev = node(vma, prev);
 970if (!prev->memory) {
 971prev->size += vma->size;
 972rb_erase(>tree, >root);
 973list_del(>head);
 974kfree(vma);
 975vma = prev;
 976}
 977}
 978
 979next = node(vma, next);
 980if (next && next->part) {
 981if (!next->memory) {
 982vma->size += next->size;
 983rb_erase(>tree, >root);
 984list_del(>head);
 985kfree(next);
 986}
 987}
 988}

The issue here is that in case _node_ returns NULL, _prev_ is not being null
checked, hence there is a potential null pointer dereference at line 970.

Notice that _next_ is being null checked at line 980, so I wonder if _prev_
should be checked the same as _next_.

The fact that both _next_ and next->part are null checked, makes me wonder
if in case _prev_ actually needs to be checked, there is another pointer
contained into _prev_ to be validated as well? I'm sorry, this is not clear
to me at this moment.

It's not checked because it can't happen.  If vma->part is set, there
will be a previous node that it was split from.



I got it.

Thanks, Ben.
--
Gustavo






___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [drm-nouveau-mmu] question about potential NULL pointer dereference

[Ben Skeggs]

On Wed, Feb 14, 2018 at 1:40 AM, Gustavo A. R. Silva
 wrote:
>
> Hi all,
>
> While doing some static analysis I ran into the following piece of code at
> drivers/gpu/drm/nouveau/nvkm/subdev/mmu/vmm.c:957:
>
>  957#define node(root, dir) ((root)->head.dir == >list) ? NULL :
> \
>  958list_entry((root)->head.dir, struct nvkm_vma, head)
>  959
>  960void
>  961nvkm_vmm_unmap_region(struct nvkm_vmm *vmm, struct nvkm_vma *vma)
>  962{
>  963struct nvkm_vma *next;
>  964
>  965nvkm_memory_tags_put(vma->memory, vmm->mmu->subdev.device,
> >tags);
>  966nvkm_memory_unref(>memory);
>  967
>  968if (vma->part) {
>  969struct nvkm_vma *prev = node(vma, prev);
>  970if (!prev->memory) {
>  971prev->size += vma->size;
>  972rb_erase(>tree, >root);
>  973list_del(>head);
>  974kfree(vma);
>  975vma = prev;
>  976}
>  977}
>  978
>  979next = node(vma, next);
>  980if (next && next->part) {
>  981if (!next->memory) {
>  982vma->size += next->size;
>  983rb_erase(>tree, >root);
>  984list_del(>head);
>  985kfree(next);
>  986}
>  987}
>  988}
>
> The issue here is that in case _node_ returns NULL, _prev_ is not being null
> checked, hence there is a potential null pointer dereference at line 970.
>
> Notice that _next_ is being null checked at line 980, so I wonder if _prev_
> should be checked the same as _next_.
>
> The fact that both _next_ and next->part are null checked, makes me wonder
> if in case _prev_ actually needs to be checked, there is another pointer
> contained into _prev_ to be validated as well? I'm sorry, this is not clear
> to me at this moment.
It's not checked because it can't happen.  If vma->part is set, there
will be a previous node that it was split from.

Ben.

>
> I appreciate your feedback
> Thank you
> --
> Gustavo
>
>
>
>
>
>
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel