linux-kernel - Re: [PATCH] drm/amdgpu: fix a missing-check bug

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 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:   Mon, 22 Oct 2018 17:51:41 +0000
From:   "Kuehling, Felix" <Felix.Kuehling@....com>
To:     Wenwen Wang <wang6495@....edu>
CC:     "Zhou, David(ChunMing)" <David1.Zhou@....com>,
        David Airlie <airlied@...ux.ie>, Kangjie Lu <kjlu@....edu>,
        open list <linux-kernel@...r.kernel.org>,
        "open list:RADEON and AMDGPU DRM DRIVERS" 
        <amd-gfx@...ts.freedesktop.org>,
        "open list:DRM DRIVERS" <dri-devel@...ts.freedesktop.org>,
        "Deucher, Alexander" <Alexander.Deucher@....com>,
        "Wang, Ken" <Ken.Wang@....com>,
        "Koenig, Christian" <Christian.Koenig@....com>
Subject: Re: [PATCH] drm/amdgpu: fix a missing-check bug

The BIOS signature check does not guarantee integrity of the BIOS image
either way. As I understand it, the signature is just a magic number.
It's not a cryptographic signature. The check is just a sanity check.
Therefore this change doesn't add any meaningful protection against the
scenario you described.

Regards,
  Felix

On 2018-10-20 4:57 p.m., Wenwen Wang wrote:
> In amdgpu_read_bios_from_rom(), the header of the VBIOS is firstly copied
> to 'header' from an IO memory region through
> amdgpu_asic_read_bios_from_rom(). Then the header is checked to see whether
> it is a valid header. If yes, the whole VBIOS, including the header, is
> then copied to 'adev->bios'. The problem here is that no check is enforced
> on the header after the second copy. Given that the device also has the
> permission to access the IO memory region, it is possible for a malicious
> device controlled by an attacker to modify the header between these two
> copies. By doing so, the attacker can supply compromised VBIOS, which can
> cause undefined behavior of the kernel and introduce potential security
> issues.
>
> This patch rewrites the header in 'adev->bios' using the header acquired in
> the first copy.
>
> Signed-off-by: Wenwen Wang <wang6495@....edu>
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_bios.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_bios.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_bios.c
> index a5df80d..ac701f4 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_bios.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_bios.c
> @@ -181,6 +181,8 @@ static bool amdgpu_read_bios_from_rom(struct amdgpu_device *adev)
>  	/* read complete BIOS */
>  	amdgpu_asic_read_bios_from_rom(adev, adev->bios, len);
>  
> +	memcpy(adev->bios, header, AMD_VBIOS_SIGNATURE_END);
> +
>  	if (!check_atom_bios(adev->bios, len)) {
>  		kfree(adev->bios);
>  		return false;