linux-kernel - Re: [PATCH 1/4] mm: introduce vma_set

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Fri, 9 Oct 2020 14:15:16 +0200
From:   Christian König <christian.koenig@....com>
To:     Jason Gunthorpe <jgg@...pe.ca>, linux-mm@...ck.org,
        linux-kernel@...r.kernel.org, linaro-mm-sig@...ts.linaro.org,
        dri-devel@...ts.freedesktop.org, linux-media@...r.kernel.org,
        chris@...is-wilson.co.uk, airlied@...hat.com,
        akpm@...ux-foundation.org, sumit.semwal@...aro.org
Subject: Re: [PATCH 1/4] mm: introduce vma_set_file function v2

Am 09.10.20 um 14:12 schrieb Jason Gunthorpe:
> On Fri, Oct 09, 2020 at 09:39:00AM +0200, Daniel Vetter wrote:
>> I just noticed this here in the patch because everyone else does not do
>> this. But looking at the mmap_region() code in mmap.c we seem to indeed
>> have this problem for the error path:
>>
>> unmap_and_free_vma:
>> 	vma->vm_file = NULL;
>> 	fput(file);
>>
>> Note that the success path does things correctly (a bit above):
>>
>> 	file = vma->vm_file;
>> out:
>>
>> So it indeed looks like dma-buf is the only one that does this fully
>> correctly. So maybe we should do a follow-up patch to change the
>> mmap_region exit code to pick up whatever vma->vm_file was set instead,
>> and fput that?
> Given that this new vma_set_file() should be the only way to
> manipulate vm_file from the mmap op, I think this reflects a bug in
> mm/mmap.c.. Should be:
>
> unmap_and_free_vma:
>          fput(vma->vm_file);
>          vma->vm_file = NULL;
>
> Then everything works the way you'd expect without tricky error
> handling

That's what Daniel suggested as well, yes.

Going to craft a separate patch for this.

Thanks,
Christian.

>
> Jason