[11:58] <mchehab_> Hi all
[11:59] <mchehab_> Im currently in transit
[11:59] <mchehab_> Internet not stable
[12:00] <mchehab_> Anything for us to discuss today?
[12:03] <sailus> One pull request to add a fix for 4.17:
[12:03] <sailus> https://patchwork.linuxtv.org/patch/48599/
[12:03] <sailus> I guess it's not urgent in any way right now.
[12:03] <sailus> Otherwise I've been mostly reviewing patches. :-)
[12:03] <sailus> Anyone else?
[12:04] <mchehab_> Yeah i saw that
[12:04] <mchehab_> I ll handle next week
[12:05] <mchehab_> Almost fimished with sparse/smatch errors
[12:05] <mchehab_> Just 2 left
[12:05] <mchehab_> On 64 bits
[12:06] <mchehab_> None on 32 bits
[12:06] <mchehab_> I ll send an email to maint list after finhshing it
[12:30] <mchehab> just one warning:
[12:30] <mchehab> drivers/media/v4l2-core/v4l2-compat-ioctl32.c:621:21: warning: incorrect type in argument 1 (different address spaces)
[12:30] <mchehab> drivers/media/v4l2-core/v4l2-compat-ioctl32.c:621:21:    expected void const volatile [noderef] <asn:1>*<noident>
[12:30] <mchehab> drivers/media/v4l2-core/v4l2-compat-ioctl32.c:621:21:    got struct v4l2_plane [noderef] <asn:1>**<noident>
[12:30] <mchehab> I'm starting to suspect that it could be a real bug
[12:30] <mchehab> the logic there seems weird
[12:31] <mchehab> 		if (get_user(uplane, ((__force struct v4l2_plane __user **)&kp->m.planes)))
[12:31] <mchehab> 			return -EFAULT;
[12:31] <mchehab> having both __force and __user there looks really weird
[12:32] <mchehab> also, IMHO, doing a get_user() to a pointer of pointer looks wrong
[12:32] <sailus> I don't think it's a bug but that casting would likely benefit from cleaning.
[12:36] <mchehab> get_user (type **)m.planes) look really weird
[12:36] <mchehab> m.planes seem to be a pointer, not a pointer of pointer
[12:37] <mchehab> I tried cleaning it already. I wasn't able to do it witout producing errors
[12:37] <mchehab> or warnings
[12:38] <sailus> The struct definition is missing __user in videodev2.h.
[12:38] <sailus> Could that be the reason why the casting was there in the first place?
[12:39] <sailus> That'd be a bug as itself.
[12:39] <sailus> There are a few other bugs like that.
[12:40] <sailus> I could send a patch.
[12:41] <sailus> I will send a patch.  :-)
[12:43] <mchehab> I guess there's something wrong at compat32
[12:43] <sailus> It's complicated, that's at least one thing.
[12:43] <mchehab> uplane is declared with __user
[12:43] <mchehab> it shouldn't, as that's the place where the Kernel value is stored
[12:45] <sailus> That's actually correct. But I'd rather rename the variables so that it'd align with kp and up used across the file.
[12:46] <sailus> Nowadays the kernel-allocated 64-bit structs are also mapped to the user process.
[12:48] <sailus> That's the arrangement after a1dfb4c48cc1e64eeb7800a27c66a6f7e88d075a.
[12:58] <sailus> Sent...
[13:02] <mchehab> fixed
[13:03] <sailus> What is fixed?
[13:03] <mchehab> sailus: IMHO, your patch will cause even more problems
[13:03] <sailus> Will it?
[13:04] <mchehab> but tests are needed
[13:04] <mchehab> the thing is that, on some cases, the compat32 should *not* use __user
[13:04] <mchehab> (when the data is copied from user
[13:05] <mchehab> see this one: https://patchwork.linuxtv.org/patch/48609/
[13:06] <mchehab> basically, it explicitly annotates with __user only where required
[13:07] <mchehab> adding __user at videodev2.h for sure require other changes to avoid smatch/sparse warnings
[13:07] <sailus> I might have had agreed with the patch before fixing CVE-2017-13166.
[13:08] <mchehab> and it will cause warnings on every place where those structs are used with a Kernel ptr
[13:08] <mchehab> (e. g. inside drivers and v4l2-core, after copy_from/to_user)
[13:08] <sailus> That one was a rather non-obvious one; as a result all memory allocated for 64-bit structs consumed by the kernel IOCTL handler in V4L2 are also mapped to the user process, hence references to that memory bear __user modifier.
[13:09] <mchehab> sorry, not following you on that
[13:09] <sailus> Either way, I'd want to see an ack from Hans to any patch that touches the compat code.
[13:10] <mchehab> basically:
[13:10] <mchehab> __user should be used *only* at v4l2-dev (where copy from/to happens) or at compat32
[13:10] <sailus> Please see: a1dfb4c48cc1e64eeb7800a27c66a6f7e88d075a
[13:10] <mchehab> sailus: I saw that patch
[13:10] <mchehab> I fixed a bug on it recently
[13:10] <sailus> Ok.
[13:11] <mchehab> it forgets doing a get_user() for overlay
[13:12] <mchehab> placing __user annotations at videodev2.h is wrong, as any other usage outside compat32/v4l2-core will cause troubles
[13:12] <mchehab> (and it should, as, after the user memory is mapped to the Kernelspace, you shouldn't use __user)
[13:12] <sailus> I see your patch removes the __user modifier from two variables that point to user-mapped memory.
[13:14] <sailus> Hmm. Right, that's a valid point, too.
[13:14] <mchehab> yes, because that cause troubles where they're already mapped to KS
[13:14] <mchehab> I actually moved the __user from the var definition to the places where it was not mapped yet
[13:16] <mchehab> on my patch, at put_v4l2_buffer32()
[13:16] <sailus> So you could do the casting either in v4l2-ctrls.c or in the compat code and base IOCTL handler. I guess it looks better where it is now.
[13:16] <mchehab> uplane is not an __user pointer after get_user()
[13:16] <sailus> But still this is visible in the public uAPI header, and that's a bit ugly, isn't it?
[13:16] <mchehab> just like compat_caddr_t p;
[13:16] <mchehab> with is also not taged as __user
[13:17] <mchehab> yes. I susepect we could get rid of all __user annotations from the public uAPI header
[13:18] <mchehab> (well, a Kernel script removes __user annotations from there - so no harm for final user is done)
[13:19] <mchehab> there is one thing that I'm unsure at put_v4l2_buffer32 code, though:
[13:19] <mchehab>     if (get_user(uplane, (__force void __user *)&kp->m.planes))
[13:19] <mchehab> I guess, it should be, instead:
[13:19] <mchehab> if (get_user(uplane, (__force void __user *)kp->m.planes))
[13:19] <mchehab> as m.planes is already a pointer
[13:22] <sailus> "&" should be there, as you need a pointer to that pointer, right.
[13:22] <sailus> Otherwise you end up accessing user-mapped memory without get_user.
[13:35] <mchehab> sailus: btw, just checked what happens with your patch:
[13:35] <mchehab> drivers/media/common/cx2341x.c:967:73: warning: incorrect type in initializer (different address spaces)
[13:35] <mchehab> drivers/media/common/cx2341x.c:967:73:    expected struct v4l2_ext_control *ctrl
[13:35] <mchehab> drivers/media/common/cx2341x.c:967:73:    got struct v4l2_ext_control [noderef] <asn:1>*
[13:35] <mchehab> (similar errors on several other drivers)
[13:36] <mchehab> the patch I sent is wrong through... not sure what I did, but I ended doing something wrong while handling it
[13:36] <mchehab> I'll have to fix and post a v2
[13:42] <sailus> I've marked mine as rejected in Patchwork.
[13:43] <mchehab> ok
[15:30] <mchehab> hmm...
[15:30] <mchehab> <laurent.pinchart+renesas@ideasonboard.com>: connect to
[15:30] <mchehab>     mail.ideasonboard.com[2001:4b98:dc2:45:216:3eff:febb:480d]:25: Network is
[15:30] <mchehab>     unreachable
[15:30] <mchehab> it seems that something got broken at pinchartl's environment