linux-kernel - Re: [PATCH 1/1] drm/ttm: Set memory as decrypted for ttm framebuffer mappings

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]

Message-ID: <42ed6188-0479-1ba2-99a0-a725286cf476@amd.com>
Date:   Thu, 27 Sep 2018 20:28:06 +0000
From:   Jiandi An <jiandi@....com>
To:     "Koenig, Christian" <Christian.Koenig@....com>,
        "An, Jiandi" <Jiandi.An@....com>,
        "dri-devel@...ts.freedesktop.org" <dri-devel@...ts.freedesktop.org>
CC:     "airlied@...ux.ie" <airlied@...ux.ie>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "Huang, Ray" <Ray.Huang@....com>,
        "Zhang, Jerry" <Jerry.Zhang@....com>
Subject: Re: [PATCH 1/1] drm/ttm: Set memory as decrypted for ttm framebuffer
 mappings



On 08/24/2018 02:21 AM, Christian König wrote:
> Am 24.08.2018 um 01:05 schrieb Jiandi An:
>>
>>
>> On 08/23/2018 01:47 AM, Christian König wrote:
>>> Am 22.08.2018 um 22:57 schrieb Jiandi An:
>>>>
>>>> On 08/22/2018 02:09 PM, Christian König wrote:
>>>>> Am 22.08.2018 um 20:57 schrieb Jiandi An:
>>>>>> Framebuffer memory needs to be accessed decrypted.  Ensure the
>>>>>> memory encryption mask is not set for the ttm framebuffer mappings.
>>>>> NAK, the memory not only needs to be decrypted while CPU accessed but all the time.
>>>>>
>>>>> ttm_page_alloc.c and ttm_page_alloc_dma.c should already take care of that while mapping the pages.
>>>>>
>>>>> Regards,
>>>>> Christian.
>>>>>
>>>> The path where the issue comes in is booting guest with AMD SEV enabled while using virtio-vga device.
>>>> The ttm->pages doesn't go through ttm_populate_and_map_pages().
>>>
>>> And that is the point where you need to jump in and fix the problem.
>>>
>>> When a driver implements the populate() and unpopulate() callbacks themselves it needs to take care of all that handling.
>> Thanks for the suggestion and guidance.  So you want me to register the callbacks something like virtio_gpu_ttm_populate() and virtio_gpu_ttm_unpopulate() in the virtio_gpu_bo_driver,
> 
> Yes, that is one possibility. The alternative is directly hack that into the ttm_tt_populate() and ttm_tt_unpopulate() functions.
> 
>> and perform mapping by calling ttm_bo_kmap()?  Then bring setting memory decrypted/encryped by calling set_memory_decrypted()/set_memory_encrypted() outside of ttm_bo_kmap(), do it within virtio_gpu_ttm_populate() and virtio_gpu_ttm_unpopulate() callbacks?
> 
> No, don't call ttm_bo_kmap() here. I think I now understand where we have a misunderstanding.
> 
> ttm_bo_kmap() just creates a vmap of the original pages to make them look linear to the kernel space.
> 
> What you need to do instead is to change the attributes of the original page mapping, not the copy in the vmap.
> 
> In other words you should call  set_memory_decrypted() for each individually allocated page.
> 
>>
>> Then get rid of the separate call of virtio_gpu_vmap_fb() the virtio_gpu driver does?
>>
>>>
>>>>
>>>> In the kernel path, the virtio_gpu driver calls virtio_gpu_alloc_object() and goes down to ttm to
>>>> allocate pages in ttm_pool_populate().  Driver in guest then does virtio_gpu_vmap_fb() which goes
>>>> down to ttm_bo_kmap_ttm() and does a vmap() for GVA to GPA for those pages.  If SEV is enabled,
>>>> decryption should be set in this GVA to GPA mapping.
>>>
>>> That assumption is what is incorrect here.
>>>
>>> TTM can't work with encrypted pages, so you need to set the pages as decrypted directly after calling ttm_pool_populate().
>>>
>>> And obviously set it to encrypted again before calling ttm_pool_unpopulate().
>>>
>>> Probably best if we add that to ttm_tt_populate()/ttm_tt_unpopulate().
>>
>> Here when you say ttm_tt_populate() and ttm_tt_unpopulate() you mean the virtio_gpu_ttm_populate() and virtio_gpu_ttm_unpopulate() that I register in virtio_gpu_bo_driver right?
> 
> Either as driver specific callbacks or directly in the fallback path of ttm_tt_populate() and ttm_tt_unpopulate().
> 

After working and testing with Gerd Hoffmann's patches, getting virtio-gpu framebuffer pages to
go through dma-api is more appropriate fix and is what we would like to do for SEV as well.

Gerd Hoffmann's patches to add iommu support for virtio-gpu in QEMU and kernel together to allow
iommu_platform attribute to be specified for virtio-gpu devices in qemu command line and the ttm
pages for virtio-gpu framebuffers to go through dma-api.  This solves the issue for SEV as SEV
forces dma-api to go to swiotlb for bounce buffer and that's set as decrypted.

Gerd Hoffmann QEMU patch
https://git.kraxel.org/cgit/qemu/commit/?h=sirius/virtio-gpu-iommu&id=588ebd02e9daedd6e1c5453ef828c064a16e43c3 
https://git.kraxel.org/cgit/qemu/commit/?h=sirius/virtio-gpu-iommu&id=86f9d30e4a44ca47f007e51ab5744f87e79fb83e 

Gerd Hoffmann Kernel patch
https://lore.kernel.org/lkml/20180829122026.27012-2-kraxel@redhat.com/
https://lore.kernel.org/lkml/20180829122026.27012-3-kraxel@redhat.com/ 

Gerd Hoffmann's kernel patch is missing dma_sync, graphical screen turns black blank screen.
Fix is the following two patches
https://lore.kernel.org/lkml/20180919113406.ukhgbiukx7vsuinw@sirius.home.kraxel.org/ 
https://lore.kernel.org/lkml/ffc08d4e-5da8-7631-438f-311fd0564864@amd.com/

- Jiandi

> Regards,
> Christian.
>