linux-kernel - Re: <Query> Looking more details and reasons for using orig_add

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 linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Thu, 23 Feb 2017 02:25:53 +0200
From:   Laurent Pinchart <laurent.pinchart@...asonboard.com>
To:     Mauro Carvalho Chehab <mchehab@...pensource.com>
Cc:     Sodagudi Prasad <psodagud@...eaurora.org>,
        James Morse <james.morse@....com>, linux-media@...r.kernel.org,
        shijie.huang@....com, catalin.marinas@....com, will.deacon@....com,
        mark.rutland@....com, akpm@...ux-foundation.org,
        sandeepa.s.prabhu@...il.com, linux-arm-kernel@...ts.infradead.org,
        linux-kernel@...r.kernel.org, hans.verkuil@...co.com,
        sakari.ailus@...ux.intel.com, tiffany.lin@...iatek.com,
        nick@...anahar.org, shuah@...nel.org, ricardo.ribalda@...il.com
Subject: Re: <Query> Looking more details and reasons for using orig_add_limit.

Hi Mauro,

On Wednesday 22 Feb 2017 17:25:41 Mauro Carvalho Chehab wrote:
> Em Wed, 22 Feb 2017 21:53:08 +0200 Laurent Pinchart escreveu:
> > On Tuesday 21 Feb 2017 06:20:58 Sodagudi Prasad wrote:
> >> Hi mchehab/linux-media,
> >> 
> >> It is not clear why KERNEL_DS was set explicitly here. In this path
> >> video_usercopy() gets  called  and it
> >> copies the “struct v4l2_buffer” struct to user space stack memory.
> >> 
> >> Can you please share reasons for setting to KERNEL_DS here?
> > 
> > It's a bit of historical hack. To implement compat ioctl handling, we copy
> > the ioctl 32-bit argument from userspace, turn it into a native 64-bit
> > ioctl argument, and call the native ioctl code. That code expects the
> > argument to be stored in userspace memory and uses get_user() and
> > put_user() to access it. As the 64-bit argument now lives in kernel
> > memory, my understanding is that we fake things up with KERNEL_DS.
> 
> Precisely. Actually, if I remember well, this was needed to pass pointer
> arguments from 32 bits userspace to 64 bits kernelspace. There are a lot of
> V4L2 ioctls that pass structures with pointers on it. Setting DS cause
> those pointers to do the right thing, but yeah, it is hackish.

We should restructure the core ioctl code to decouple copy from/to user and 
ioctl execution (this might just be a matter of exporting a currently static 
function), and change the compat code to perform the copy/from to user 
directly when converting between 32-bit and 64-bit structures (dropping all 
the alloc in userspace hacks) and call the ioctl execution handler. That will 
fix the problem. Any volunteer ? :-)

> This used to work fine on x86_64 (when such code was written e. g. Kernel
> 2.6.1x). I never tested myself on ARM64, but I guess it used to work, as we
> received some patches fixing support for some ioctl compat code due to
> x86_64/arm64 differences in the past.
> 
> On what Kernel version it started to cause troubles? 4.9? If so, then
> maybe the breakage is a side effect of VM stack changes.
> 
> > The ioctl code should be refactored to get rid of this hack.
> 
> Agreed.

-- 
Regards,

Laurent Pinchart