Re: [PATCH] media: v4l2-compat-ioctl32: don't oops on overlay

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Thu, Mar 29, 2018 at 09:19:43AM +0300, Laurent Pinchart wrote:
> Hello,
> 
> On Wednesday, 28 March 2018 23:16:08 EEST Sakari Ailus wrote:
> > On Wed, Mar 28, 2018 at 02:59:22PM -0300, Mauro Carvalho Chehab wrote:
> > > At put_v4l2_window32(), it tries to access kp->clips. However,
> > > kp points to an userspace pointer. So, it should be obtained
> > > 
> > > via get_user(), otherwise it can OOPS:
> > >  vivid-000: ==================  END STATUS  ==================
> > >  BUG: unable to handle kernel paging request at 00000000fffb18e0
> > >  IP: [<ffffffffc05468d9>] __put_v4l2_format32+0x169/0x220 [videodev]
> > >  PGD 3f5776067 PUD 3f576f067 PMD 3f5769067 PTE 800000042548f067
> > >  Oops: 0001 [#1] SMP
> > >  Modules linked in: vivid videobuf2_vmalloc videobuf2_memops
> > >  v4l2_dv_timings videobuf2_core v4l2_common videodev media xt_CHECKSUM
> > >  iptable_mangle ipt_MASQUERADE nf_nat_masquerade_ipv4 iptable_nat
> > >  nf_nat_ipv4 nf_nat nf_conntrack_ipv4 nf_defrag_ipv4 xt_conntrack
> > >  nf_conntrack tun bridge stp llc ebtable_filter ebtables ip6table_filter
> > >  ip6_tables bluetooth rfkill binfmt_misc snd_hda_codec_hdmi i915
> > >  snd_hda_intel snd_hda_controller snd_hda_codec intel_rapl
> > >  x86_pkg_temp_thermal snd_hwdep intel_powerclamp snd_pcm coretemp
> > >  snd_seq_midi kvm_intel kvm snd_seq_midi_event snd_rawmidi i2c_algo_bit
> > >  drm_kms_helper snd_seq drm crct10dif_pclmul e1000e snd_seq_device
> > >  crc32_pclmul snd_timer ghash_clmulni_intel snd mei_me mei ptp pps_core
> > >  soundcore lpc_ich video crc32c_intel [last unloaded: media] CPU: 2 PID:
> > >  28332 Comm: v4l2-compliance Not tainted 3.18.102+ #107 Hardware name:   
> > >                /NUC5i7RYB, BIOS RYBDWi35.86A.0364.2017.0511.0949
> > >  05/11/2017 task: ffff8804293f8000 ti: ffff8803f5640000 task.ti:
> > >  ffff8803f5640000 RIP: 0010:[<ffffffffc05468d9>]  [<ffffffffc05468d9>]
> > >  __put_v4l2_format32+0x169/0x220 [videodev] RSP: 0018:ffff8803f5643e28 
> > >  EFLAGS: 00010246
> > >  RAX: 0000000000000000 RBX: 0000000000000000 RCX: 00000000fffb1ab4
> > >  RDX: 00000000fffb1a68 RSI: 00000000fffb18d8 RDI: 00000000fffb1aa8
> > >  RBP: ffff8803f5643e48 R08: 0000000000000001 R09: ffff8803f54b0378
> > >  R10: 0000000000000000 R11: 0000000000000168 R12: 00000000fffb18c0
> > >  R13: 00000000fffb1a94 R14: 00000000fffb18c8 R15: 0000000000000000
> > >  FS:  0000000000000000(0000) GS:ffff880456d00000(0063)
> > >  knlGS:00000000f7100980 CS:  0010 DS: 002b ES: 002b CR0: 0000000080050033
> > >  CR2: 00000000fffb18e0 CR3: 00000003f552b000 CR4: 00000000003407e0
> > >  
> > >  Stack:
> > >   00000000fffb1a94 00000000c0cc5640 0000000000000056 ffff8804274f3600
> > >   ffff8803f5643ed0 ffffffffc0547e16 0000000000000003 ffff8803f5643eb0
> > >   ffffffff81301460 ffff88009db44b01 ffff880441942520 ffff8800c0d05640
> > >  
> > >  Call Trace:
> > >   [<ffffffffc0547e16>] v4l2_compat_ioctl32+0x12d6/0x1b1d [videodev]
> > >   [<ffffffff81301460>] ? file_has_perm+0x70/0xc0
> > >   [<ffffffff81252a2c>] compat_SyS_ioctl+0xec/0x1200
> > >   [<ffffffff8173241a>] sysenter_dispatch+0x7/0x21
> > >  
> > >  Code: 00 00 48 8b 80 48 c0 ff ff 48 83 e8 38 49 39 c6 0f 87 2b ff ff ff
> > >  49 8d 45 1c e8 a3 ce e3 c0 85 c0 0f 85 1a ff ff ff 41 8d 40 ff <4d> 8b
> > >  64 24 20 41 89 d5 48 8d 44 40 03 4d 8d 34 c4 eb 15 0f 1f RIP 
> > >  [<ffffffffc05468d9>] __put_v4l2_format32+0x169/0x220 [videodev] RSP
> > >  <ffff8803f5643e28>
> > >  CR2: 00000000fffb18e0
> > > 
> > > Tested with vivid driver on Kernel v3.18.102.
> > > 
> > > Same bug happens upstream too:
> > >  BUG: KASAN: user-memory-access in __put_v4l2_format32+0x98/0x4d0
> > >  [videodev]
> > >  Read of size 8 at addr 00000000ffe48400 by task v4l2-compliance/8713
> > >  
> > >  CPU: 0 PID: 8713 Comm: v4l2-compliance Not tainted 4.16.0-rc4+ #108
> > >  Hardware name:  /NUC5i7RYB, BIOS RYBDWi35.86A.0364.2017.0511.0949
> > >  05/11/2017>  
> > >  Call Trace:
> > >   dump_stack+0x5c/0x7c
> > >   kasan_report+0x164/0x380
> > >   ? __put_v4l2_format32+0x98/0x4d0 [videodev]
> > >   __put_v4l2_format32+0x98/0x4d0 [videodev]
> > >   v4l2_compat_ioctl32+0x1aec/0x27a0 [videodev]
> > >   ? __fsnotify_inode_delete+0x20/0x20
> > >   ? __put_v4l2_format32+0x4d0/0x4d0 [videodev]
> > >   compat_SyS_ioctl+0x646/0x14d0
> > >   ? do_ioctl+0x30/0x30
> > >   do_fast_syscall_32+0x191/0x3f4
> > >   entry_SYSENTER_compat+0x6b/0x7a
> > >  
> > >  ==================================================================
> > >  Disabling lock debugging due to kernel taint
> > >  BUG: unable to handle kernel paging request at 00000000ffe48400
> > >  IP: __put_v4l2_format32+0x98/0x4d0 [videodev]
> > >  PGD 3a22fb067 P4D 3a22fb067 PUD 39b6f0067 PMD 39b6f1067 PTE
> > >  80000003256af067 Oops: 0001 [#1] SMP KASAN
> > >  Modules linked in: vivid videobuf2_vmalloc videobuf2_dma_contig
> > >  videobuf2_memops v4l2_tpg v4l2_dv_timings videobuf2_v4l2
> > >  videobuf2_common v4l2_common videodev xt_CHECKSUM iptable_mangle
> > >  ipt_MASQUERADE nf_nat_masquerade_ipv4 iptable_nat nf_nat_ipv4 nf_nat
> > >  nf_conntrack_ipv4 nf_defrag_ipv4 xt_conntrack nf_conntrack libcrc32c tun
> > >  bridge stp llc ebtable_filter ebtables ip6table_filter ip6_tables
> > >  bluetooth rfkill ecdh_generic binfmt_misc snd_hda_codec_hdmi intel_rapl
> > >  x86_pkg_temp_thermal intel_powerclamp i915 coretemp snd_hda_intel
> > >  snd_hda_codec kvm_intel snd_hwdep snd_hda_core kvm snd_pcm irqbypass
> > >  crct10dif_pclmul crc32_pclmul snd_seq_midi ghash_clmulni_intel
> > >  snd_seq_midi_event i2c_algo_bit intel_cstate snd_rawmidi intel_uncore
> > >  snd_seq drm_kms_helper e1000e snd_seq_device snd_timer intel_rapl_perf>  
> > >   drm ptp snd mei_me mei lpc_ich pps_core soundcore video crc32c_intel
> > >  
> > >  CPU: 0 PID: 8713 Comm: v4l2-compliance Tainted: G    B           
> > >  4.16.0-rc4+ #108 Hardware name:  /NUC5i7RYB, BIOS
> > >  RYBDWi35.86A.0364.2017.0511.0949 05/11/2017 RIP:
> > >  0010:__put_v4l2_format32+0x98/0x4d0 [videodev]
> > >  RSP: 0018:ffff8803b9be7d30 EFLAGS: 00010282
> > >  RAX: 0000000000000000 RBX: ffff8803ac983e80 RCX: ffffffff8cd929f2
> > >  RDX: 1ffffffff1d0a149 RSI: 0000000000000297 RDI: 0000000000000297
> > >  RBP: 00000000ffe485c0 R08: fffffbfff1cf5123 R09: ffffffff8e7a8948
> > >  R10: 0000000000000001 R11: fffffbfff1cf5122 R12: 00000000ffe483e0
> > >  R13: 00000000ffe485c4 R14: ffff8803ac985918 R15: 00000000ffe483e8
> > >  FS:  0000000000000000(0000) GS:ffff880407400000(0063)
> > >  knlGS:00000000f7a46980 CS:  0010 DS: 002b ES: 002b CR0: 0000000080050033
> > >  CR2: 00000000ffe48400 CR3: 00000003a83f2003 CR4: 00000000003606f0
> > >  
> > >  Call Trace:
> > >   v4l2_compat_ioctl32+0x1aec/0x27a0 [videodev]
> > >   ? __fsnotify_inode_delete+0x20/0x20
> > >   ? __put_v4l2_format32+0x4d0/0x4d0 [videodev]
> > >   compat_SyS_ioctl+0x646/0x14d0
> > >   ? do_ioctl+0x30/0x30
> > >   do_fast_syscall_32+0x191/0x3f4
> > >   entry_SYSENTER_compat+0x6b/0x7a
> > >  
> > >  Code: 4c 89 f7 4d 8d 7c 24 08 e8 e6 a4 69 cb 48 8b 83 98 1a 00 00 48 83
> > >  e8 10 49 39 c7 0f 87 9d 01 00 00 49 8d 7c 24 20 e8 c8 a4 69 cb <4d> 8b
> > >  74 24 20 4c 89 ef 4c 89 fe ba 10 00 00 00 e8 23 d9 08 cc RIP:
> > >  __put_v4l2_format32+0x98/0x4d0 [videodev] RSP: ffff8803b9be7d30 CR2:
> > >  00000000ffe48400
> > > 
> > > cc: stable@xxxxxxxxxxxxxxx
> > > Signed-off-by: Mauro Carvalho Chehab <mchehab@xxxxxxxxxxxxxxxx>
> > > ---
> > > 
> > >  drivers/media/v4l2-core/v4l2-compat-ioctl32.c | 4 +++-
> > >  1 file changed, 3 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
> > > b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c index
> > > 5198c9eeb348..4312935f1dfc 100644
> > > --- a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
> > > +++ b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
> > > @@ -101,7 +101,7 @@ static int get_v4l2_window32(struct v4l2_window __user
> > > *kp,
> > >  static int put_v4l2_window32(struct v4l2_window __user *kp,
> > >  			     struct v4l2_window32 __user *up)
> > >  {
> > > -	struct v4l2_clip __user *kclips = kp->clips;
> > > +	struct v4l2_clip __user *kclips;
> > >  	struct v4l2_clip32 __user *uclips;
> > >  	compat_caddr_t p;
> > >  	u32 clipcount;
> > > @@ -116,6 +116,8 @@ static int put_v4l2_window32(struct v4l2_window __user
> > > *kp,
> > >  	if (!clipcount)
> > >  		return 0;
> > > 
> > > +	if (get_user(kclips, &kp->clips))
> > > +		return -EFAULT;
> > >  	if (get_user(p, &up->clips))
> > >  		return -EFAULT;
> > >  	uclips = compat_ptr(p);
> > 
> > Good find. I checked for similar problems elsewhere in the file but could
> > not find any.
> > 
> > Reviewed-by: Sakari Ailus <sakari.ailus@xxxxxxxxxxxxxxx>
> 
> Would it be useful to rename kp to something that doesn't imply the memory is 
> kernel memory ? I was confused at first when reading this patch.

It's allocated by the kernel but nowadays that's done using
compat_alloc_user_space() so it's mapped to the user as well. The same
practice applies to the whole file. It's a bit confusing when you first see
it, I have to admit.

The 32-bit up comes from the user but the 64-bit is actually used by the
real IOCTL handler. Compat and native? They're longer though.

Anyway that change would be a separate patch.

-- 
Sakari Ailus
sakari.ailus@xxxxxxxxxxxxxxx



[Index of Archives]     [Linux Input]     [Video for Linux]     [Gstreamer Embedded]     [Mplayer Users]     [Linux USB Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]
  Powered by Linux