Re: BUG: unable to handle kernel NULL pointer dereference (drm_getunique)
From: Johannes Weiner
Date: Thu Jun 05 2008 - 14:05:49 EST
Hi,
Dave Airlie <airlied@xxxxxxxxxx> writes:
>> Make that run-time tested, also. This does not just affect linux-next.
>> The following program oopses my box running Linus' current git:
>>
>> #include <fcntl.h>
>> #include <unistd.h>
>> #include <sys/ioctl.h>
>>
>> #include <drm/drm.h>
>>
>> #define DRM_DFILE "/dev/dri/card0"
>> #define DRM_IOCTL_CAPUT _IO(DRM_IOCTL_BASE, 0x01)
>>
>> int main(void)
>> {
>> int fd = open(DRM_DFILE, O_RDONLY);
>>
>> if (fd < 0)
>> return 1;
>>
>> ioctl(fd, DRM_IOCTL_CAPUT);
>> perror("ioctl");
>> close(fd);
>> return 0;
>> }
>>
>> ->
>>
>> [ 714.581505] BUG: unable to handle kernel NULL pointer dereference at 00000000
>> [ 714.583459] IP: [<c027d491>] drm_getunique+0x11/0x40
>> [ 714.585460] *pde = 00000000
>> [ 714.587389] Oops: 0000 [#1] PREEMPT
>> [ 714.588271] Modules linked in: snd_intel8x0 snd_ac97_codec ac97_bus snd_pcm snd_page_alloc
>> [ 714.588271]
>> [ 714.588271] Pid: 1339, comm: break-drm Not tainted (2.6.26-rc4-00232-gedeb280 #43)
>> [ 714.588271] EIP: 0060:[<c027d491>] EFLAGS: 00010282 CPU: 0
>> [ 714.588271] EIP is at drm_getunique+0x11/0x40
>> [ 714.588271] EAX: cf861000 EBX: 00000000 ECX: 00000000 EDX: 00000000
>> [ 714.588271] ESI: cf861000 EDI: cfa87700 EBP: 00000000 ESP: cecc3f20
>> [ 714.588271] DS: 007b ES: 007b FS: 0000 GS: 0033 SS: 0068
>> [ 714.588271] Process break-drm (pid: 1339, ti=cecc2000 task=cf9a6dc0 task.ti=cecc2000)
>> [ 714.588271] Stack: c027d480 cf861000 c027c592 ced53b7c b7fc00d0 c0159f3b ced53b7c 000000ce
>> [ 714.588271] 00000000 00000000 00006401 00000000 cf861024 c04730a0 ced33700 bf965788
>> [ 714.588271] 00006401 c017aea8 bf965788 ced33700 ced33700 00000003 cecc2000 c017b0fb
>> [ 714.588271] Call Trace:
>> [ 714.588271] [<c027d480>] drm_getunique+0x0/0x40
>> [ 714.588271] [<c027c592>] drm_ioctl+0x1a2/0x2e0
>> [ 714.588271] [<c0159f3b>] handle_mm_fault+0xeb/0x5e0
>> [ 714.588271] [<c017aea8>] vfs_ioctl+0x78/0x90
>> [ 714.588271] [<c017b0fb>] do_vfs_ioctl+0x23b/0x2c0
>> [ 714.588271] [<c016ce2a>] do_sys_open+0xba/0xe0
>> [ 714.588271] [<c017b1bd>] sys_ioctl+0x3d/0x70
>> [ 714.588271] [<c0103152>] syscall_call+0x7/0xb
>> [ 714.588271] =======================
>> [ 714.588271] Code: 8b 04 24 e8 e2 1b 12 00 31 c0 5b 5b 5e 5f 5d c3 8d 76 00 8d bc 27 00 00 00 00 83 ec 08 89 1c 24 89 d3 89 74 24 04 89 c6 8b 48 04 <39> 0a 73 11 89 0b 31 d2 8b 1c 24 89 d0 8b 74 24 04 83 c4 08 c3
>> [ 714.588271] EIP: [<c027d491>] drm_getunique+0x11/0x40 SS:ESP 0068:cecc3f20
>> [ 714.659745] ---[ end trace fd8339ca8c62cb63 ]---
>> [ 714.663840] [drm:drm_release] *ERROR* Device busy: 1 0
>>
>> My patch fixes this, but here is a better one that checks the whole cmd
>> against the allowed one. Otherwise, it would be possible to trap the
>> kernel into allocating 2^15-1 bytes through kmalloc, too, at least on
>> this configuration here with _IOC_SIZEBITS == 14.
>>
>> ---
>> drm: sanity-check ioctl request
>>
>> drm_ioctl looks up a dispatcher function for special ioctls but it does
>> not precheck the commands against the registered ones, allowing users to
>> pass bogus values and potentially oops the kernel, as happened to
>> Sitsofe Wheeler who reported this bug.
>>
>> This patch checks the incoming ioctl command against the defined ones
>> before dispatching to the handler.
>>
>
>
> This patch will break some other parts of the DRM interface, I'll try
> and come up with one that is less likely do stop all the drivers
> working.
We do not want that to happen, of course :)
> The problem is there are binaries out there with bad userspace ioctl
> bits..
>
> What I'll change it to do is at least allocate the size from the kernel
> definition not what the user passed in.
>
> The big problem we have with malformed requests is that userspace asks
> for one thing and expects another, so I should key the userspace from
> the ioctl nr and use the kernel side definitions to decide the other
> bits. Clearly the drm code isn't doing that now.
Hm, like this?
diff --git a/drivers/char/drm/drm_drv.c b/drivers/char/drm/drm_drv.c
index fc54140..019bf1f 100644
--- a/drivers/char/drm/drm_drv.c
+++ b/drivers/char/drm/drm_drv.c
@@ -475,6 +475,9 @@ int drm_ioctl(struct inode *inode, struct file *filp,
else
goto err_i1;
+ /* Do not trust userspace, use our own definition */
+ cmd = ioctl->cmd;
+
func = ioctl->func;
/* is there a local override? */
if ((nr == DRM_IOCTL_NR(DRM_IOCTL_DMA)) && dev->driver->dma_ioctl)
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/