linux-kernel - Re: [PATCH v9 13/27] gunyah: vm_mgr: Add/remove user memory regions

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <c64ee823-33bd-1b89-0f17-4e73901ab40d@quicinc.com>
Date:   Mon, 30 Jan 2023 13:46:30 -0800
From:   Elliot Berman <quic_eberman@...cinc.com>
To:     Srivatsa Vaddagiri <quic_svaddagi@...cinc.com>
CC:     Bjorn Andersson <quic_bjorande@...cinc.com>,
        Alex Elder <elder@...aro.org>,
        Murali Nalajala <quic_mnalajal@...cinc.com>,
        Trilok Soni <quic_tsoni@...cinc.com>,
        Carl van Schaik <quic_cvanscha@...cinc.com>,
        Prakruthi Deepak Heragu <quic_pheragu@...cinc.com>,
        Dmitry Baryshkov <dmitry.baryshkov@...aro.org>,
        Arnd Bergmann <arnd@...db.de>,
        "Greg Kroah-Hartman" <gregkh@...uxfoundation.org>,
        Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        Jonathan Corbet <corbet@....net>,
        Bagas Sanjaya <bagasdotme@...il.com>,
        Catalin Marinas <catalin.marinas@....com>,
        Will Deacon <will@...nel.org>, Marc Zyngier <maz@...nel.org>,
        Jassi Brar <jassisinghbrar@...il.com>,
        Sudeep Holla <sudeep.holla@....com>,
        <linux-arm-msm@...r.kernel.org>, <devicetree@...r.kernel.org>,
        <linux-kernel@...r.kernel.org>, <linux-doc@...r.kernel.org>,
        <linux-arm-kernel@...ts.infradead.org>
Subject: Re: [PATCH v9 13/27] gunyah: vm_mgr: Add/remove user memory regions



On 1/25/2023 5:34 AM, Srivatsa Vaddagiri wrote:
> * Elliot Berman <quic_eberman@...cinc.com> [2023-01-20 14:46:12]:
> 
>> +	/* Check for overlap */
>> +	list_for_each_entry(tmp_mapping, &ghvm->memory_mappings, list) {
>> +		if (!((mapping->guest_phys_addr + (mapping->npages << PAGE_SHIFT) <=
>> +			tmp_mapping->guest_phys_addr) ||
>> +			(mapping->guest_phys_addr >=
>> +			tmp_mapping->guest_phys_addr + (tmp_mapping->npages << PAGE_SHIFT)))) {
>> +			ret = -EEXIST;
>> +			goto unlock;
>> +		}
>> +	}
>> +
>> +	list_add(&mapping->list, &ghvm->memory_mappings);
> 
> I think the potential race condition described last time is still possible. Pls
> check.
> 
>> +unlock:
>> +	mutex_unlock(&ghvm->mm_lock);
>> +	if (ret)
>> +		goto free_mapping;
>> +
>> +	mapping->pages = kcalloc(mapping->npages, sizeof(*mapping->pages), GFP_KERNEL);
>> +	if (!mapping->pages) {
>> +		ret = -ENOMEM;
>> +		goto reclaim;
> 
> Same comment as last time. Can you check this error path? We seem to call
> unpin_user_page() in this path, which is not correct.
> 
>> +	}
>> +
>> +	pinned = pin_user_pages_fast(region->userspace_addr, mapping->npages,
>> +					FOLL_WRITE | FOLL_LONGTERM, mapping->pages);
>> +	if (pinned < 0) {
>> +		ret = pinned;
>> +		goto reclaim;
> 
> Same comment as above.
> 

Caught the race condition you are describing between adding and removing 
the memory parcels. Should be fixed for v10.

- Elliot