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
| ||
|
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;
Powered by blists - more mailing lists