From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754589AbdBPKjg (ORCPT ); Thu, 16 Feb 2017 05:39:36 -0500 Received: from foss.arm.com ([217.140.101.70]:39820 "EHLO foss.arm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754108AbdBPKjf (ORCPT ); Thu, 16 Feb 2017 05:39:35 -0500 Message-ID: <58A58162.2020101@arm.com> Date: Thu, 16 Feb 2017 10:39:30 +0000 From: James Morse User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Icedove/31.6.0 MIME-Version: 1.0 To: Sodagudi Prasad CC: shijie.huang@arm.com, catalin.marinas@arm.com, will.deacon@arm.com, mark.rutland@arm.com, akpm@linux-foundation.org, sandeepa.s.prabhu@gmail.com, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, mchehab@s-opensource.com Subject: Re: Looking more details and reasons for using orig_add_limit. References: <58A4450C.3040602@arm.com> <7c727e6043e58077d143e35de0ce632c@codeaurora.org> In-Reply-To: <7c727e6043e58077d143e35de0ce632c@codeaurora.org> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Prasad, On 15/02/17 21:12, Sodagudi Prasad wrote: > On 2017-02-15 04:09, James Morse wrote: >> On 15/02/17 05:52, Sodagudi Prasad wrote: >>> that driver is calling set_fs(KERNEL_DS) and then copy_to_user() to user space >>> memory. >> >> Don't do this, its exactly the case PAN+UAO and the code you pointed to are >> designed to catch. Accessing userspace needs doing carefully, setting USER_DS >> and using the put_user()/copy_to_user() accessors are the required steps. >> >> Which driver is doing this? Is it in mainline? > > Yes. It is mainline driver - drivers/media/v4l2-core/v4l2-compat-ioctl32.c > In some v4l2 use-case kernel panic is observed. Below part > of the code has set_fs to KERNEL_DS before calling native_ioctl(). > > static long do_video_ioctl(struct file *file, unsigned int cmd, unsigned long arg) > { > … > … > if (compatible_arg) > err = native_ioctl(file, cmd, (unsigned long)up); > else { > mm_segment_t old_fs = get_fs(); > > set_fs(KERNEL_DS); ====> KERNEL_DS. > err = native_ioctl(file, cmd, (unsigned long)&karg); > set_fs(old_fs); > } > > Here is the call stack which is resulting crash, because user space memory has > read only permissions. > [27249.920041] [] __arch_copy_to_user+0x110/0x180 > [27249.920047] [] video_ioctl2+0x38/0x44 > [27249.920054] [] v4l2_ioctl+0x78/0xb4 > [27249.920059] [] do_video_ioctl+0x91c/0x1160 > [27249.920064] [] v4l2_compat_ioctl32+0x60/0xcc > [27249.920071] [] compat_SyS_ioctl+0x124/0xd88 > [27249.920077] [] el0_svc_naked+0x24/0x2 It's not totally clear to me what is going on here, but some observations: the ioctl is trying to copy_to_user() to some read-only memory. This would normally fail gracefully with -EFAULT, but because KERNEL_DS has been set, the kernel checks this before calling the fault handler and calls die() on your ioctl(). The ioctl code is doing this deliberately as a compat mechanism, but the code behind file->f_op->unlocked_ioctl() expects fs==USER_DS when it does its work. That code needs to be made aware of this compat translation, or a compat_ioctl call provided. Which v4l driver is this? Which ioctl is being called? Does the driver using the v4l framework have a compat_ioctl() call? What path does this call take through v4l2_compat_ioctl32()? It looks like compat_ioctl will be skipped in certain cases, v4l2_compat_ioctl32() has: > if (_IOC_TYPE(cmd) == 'V' && _IOC_NR(cmd) < BASE_VIDIOC_PRIVATE) > ret = do_video_ioctl(file, cmd, arg); > else if (vdev->fops->compat_ioctl32) > ret = vdev->fops->compat_ioctl32(file, cmd, arg); Is your ioctl matched by that top if()? >>> If there is permission fault for user space address the above condition >>> is leading to kernel crash. Because orig_add_limit is having KERNEL_DS as set_fs >>> called before copy_to_user(). >>> >>> 1) So I would like to understand that, is that user space pointer leading to >>> permission fault not correct(condition_1) in this scenario? >> >> The correct thing has happened here. To access user space set_fs(USER_DS) first. >> (and set it back to whatever it was afterwards). >> > > So, Any clean up needed to above call path similar to what was done in the below > commit? > commit a7f61e89af73e9bf760826b20dba4e637221fcb9 - compat_ioctl: don't call > do_ioctl under set_fs(KERNEL_DS) That's clever. Is that code doing a conversion, or do you have a compat_ioctl() in your driver? It's possible that fs/compat_ioctl.c has done this work, but do_video_ioctl() un-does it. Someone who knows about v4l and compat-ioctls should take a look... This looks like a case of: > The accidental invocation of an unlocked_ioctl handler that unexpectedly > calls copy_to_user could be a severe security issue. that Jann describes in the commit message. Fixing the code behind file->f_op->unlocked_ioctl() to consider compat calls from do_video_ioctl() is one way to solve this. Thanks, James