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] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 22 Feb 2017 17:25:41 -0300
From:   Mauro Carvalho Chehab <mchehab@...pensource.com>
To:     Laurent Pinchart <laurent.pinchart@...asonboard.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.

Em Wed, 22 Feb 2017 21:53:08 +0200
Laurent Pinchart <laurent.pinchart@...asonboard.com> escreveu:

> Hi Prasad,
> 
> 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.

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.

Thanks,
Mauro