Re: [PATCH] drm/radeon: fix a missing-check bug
Am 18.10.18 um 19:13 schrieb Wenwen Wang: > In radeon_read_bios(), the bios rom is firstly mapped to the IO memory > region 'bios' through pci_map_rom(). Then the first two bytes of 'bios' are > copied to 'val1' and 'val2' respectively through readb(). After that, > 'val1' and 'val2' are checked to see whether they have expected values, > i.e., 0x55 and 0xaa, respectively. If yes, the whole data in 'bios' is then > copied to 'rdev->bios' through memcpy_fromio(). Obviously, the first two > bytes in 'bios' are copied twice. More importantly, no check is enforced on > the first two bytes of 'rdev->bios' after memcpy_fromio(). Given that the > IO memory region can also be accessed by the device, it is possible that a > malicious device can race to modify these two bytes between the two copies > and thus after memcpy_fromio(), the first two bytes in 'rdev->bios' can > have unexpected values. This can cause undefined behavior of the kernel > and introduce potential security risk, if the device can be controlled by > attackers. > > This patch rewrites the first two bytes of 'rdev->bios' after > memcpy_fromio() with expected values. Through this way, the above issue can > be avoided. Well NAK, that doesn't make any sense to me. First of all we don't map VRAM, but rather the ROM which is a read only flash. Writing to the flash is not supported through the BAR as far as I know. Then we check the first two bytes to make sure that the ROM is correctly mapped and not to prevent any malicious attacks. Regards, Christian. > > Signed-off-by: Wenwen Wang > --- > drivers/gpu/drm/radeon/radeon_bios.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/drivers/gpu/drm/radeon/radeon_bios.c > b/drivers/gpu/drm/radeon/radeon_bios.c > index 04c0ed4..f336719 100644 > --- a/drivers/gpu/drm/radeon/radeon_bios.c > +++ b/drivers/gpu/drm/radeon/radeon_bios.c > @@ -98,6 +98,8 @@ static bool radeon_read_bios(struct radeon_device *rdev) > return false; > } > memcpy_fromio(rdev->bios, bios, size); > + rdev->bios[0] = val1; > + rdev->bios[1] = val2; > pci_unmap_rom(rdev->pdev, bios); > return true; > } ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
[PATCH] drm/radeon: fix a missing-check bug
In radeon_read_bios(), the bios rom is firstly mapped to the IO memory region 'bios' through pci_map_rom(). Then the first two bytes of 'bios' are copied to 'val1' and 'val2' respectively through readb(). After that, 'val1' and 'val2' are checked to see whether they have expected values, i.e., 0x55 and 0xaa, respectively. If yes, the whole data in 'bios' is then copied to 'rdev->bios' through memcpy_fromio(). Obviously, the first two bytes in 'bios' are copied twice. More importantly, no check is enforced on the first two bytes of 'rdev->bios' after memcpy_fromio(). Given that the IO memory region can also be accessed by the device, it is possible that a malicious device can race to modify these two bytes between the two copies and thus after memcpy_fromio(), the first two bytes in 'rdev->bios' can have unexpected values. This can cause undefined behavior of the kernel and introduce potential security risk, if the device can be controlled by attackers. This patch rewrites the first two bytes of 'rdev->bios' after memcpy_fromio() with expected values. Through this way, the above issue can be avoided. Signed-off-by: Wenwen Wang --- drivers/gpu/drm/radeon/radeon_bios.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/drivers/gpu/drm/radeon/radeon_bios.c b/drivers/gpu/drm/radeon/radeon_bios.c index 04c0ed4..f336719 100644 --- a/drivers/gpu/drm/radeon/radeon_bios.c +++ b/drivers/gpu/drm/radeon/radeon_bios.c @@ -98,6 +98,8 @@ static bool radeon_read_bios(struct radeon_device *rdev) return false; } memcpy_fromio(rdev->bios, bios, size); + rdev->bios[0] = val1; + rdev->bios[1] = val2; pci_unmap_rom(rdev->pdev, bios); return true; } -- 2.7.4 ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
[PATCH] drm/radeon: fix a missing-check bug
In igp_read_bios_from_vram(), the start of vram is firstly remapped to the IO memory region 'bios' through ioremap(). Then the size and values of 'bios' are checked. For example, 'bios[0]' is compared against 0x55 and 'bios[1]' is compared against 0xaa. If no error happens during this checking process, the whole data in 'bios' is then copied to 'rdev->bios' through memcpy_fromio(). The problem here is that the checks are performed on 'bios' directly. Given that the IO memory region can also be accessed by the device, it is possible that a malicious device race to modify 'bios[0]' and/or 'bios[1]' after the checks but before memcpy_fromio(). This can cause undefined behavior of the kernel and potentially introduce security risk, especially when the device can be controlled by attackers. This patch avoids the above issue by rewriting the first two bytes of 'rdev->bios' after memcpy_fromio() with expected values. Signed-off-by: Wenwen Wang --- drivers/gpu/drm/radeon/radeon_bios.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/drivers/gpu/drm/radeon/radeon_bios.c b/drivers/gpu/drm/radeon/radeon_bios.c index 04c0ed4..d8304fa 100644 --- a/drivers/gpu/drm/radeon/radeon_bios.c +++ b/drivers/gpu/drm/radeon/radeon_bios.c @@ -69,6 +69,8 @@ static bool igp_read_bios_from_vram(struct radeon_device *rdev) return false; } memcpy_fromio(rdev->bios, bios, size); + rdev->bios[0] = 0x55; + rdev->bios[1] = 0xaa; iounmap(bios); return true; } -- 2.7.4 ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
