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

[Kuehling, Felix]

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@xxxxxxx>
> ---
>  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;