Andy Walls wrote:
(Sorry, I've probably screwed up the threading on this)
> Dr. David Alan Gilbert wrote:
> > Hi,
> > Sparse pointed me at the following line in ivtv-fileops.c's ivtv_v4l2_write:
> >
> > ivtv_write_vbi(itv, (const struct v4l2_sliced_vbi_data *)user_buf, elems);
> >
> > Now user_buf is a parameter:
> > const char __user *user_buf,
> >
> > so that is losing the __user, and I don't see what else protects
> > the accesses that ivtv_write_vbi performs.
>
> Ummm, "__user" and similar attributes like "__iomem", don't protect
> anything -- do they? I thought they were there to help tools like
> sparse to detect type problems.
That's right; hence the question.
> It looks like the patch that added "__user" attributes in the ivtv
> driver missed or deliberately ignored this function.
>
> > Is there something that makes this safe?
>
> Not from what I can see in a few minutes worth of looking.
Same here.
> >From include/linux/compiler.h"
>
> #ifdef __CHECKER__
> # define __user __attribute__((noderef, address_space(1)))
> # define __kernel __attribute__((address_space(0)))
> ...
> # define __iomem __attribute__((noderef, address_space(2)))
>
> It would appear that directly dereferencing the user pointer is not a
> good thing to do. However, as you note, that's exactly what
> ivtv_write_vbi() does.
>
> v4l2_write() and ivtv_v4l2_write() are wrappers that don't do any
> copy_from_user() calls before passing the data along.
>
> Why does the call not induce faults for people? I'm not sure. Without
> looking too hard through all the copy_from_user() checks, I'm guessing:
Well as long as the page is mapped I think the access will just work without
the copy_from_user, the problem is mainly when someone puts a dodgy pointer in.
>
> a. the user_buf for the VBI data is likely allocated properly aligned on
> a writeable page.
>
> b. 23 lines of VBI data for two fields only takes up 64*23*2 = 2944
> bytes which likely does not cross a 4096 byte page boundary
>
> c. Not many people have PVR-350's and even less use it to send out VBI
> data to their TV.
>
> Patches welcome. :)
I'm uncomfortable patching that code since I don't really know how it's
supposed to work and don't have any of the hardware to test it with.
However, I think what it needs is:
*) Since ivtv_write_vbi takes an array of those structures it's not
easy to copy it at the point of the call in ivtv_v4l2_write, so I think
the right thing is to change ivtv_write_vbi to take a __user pointer and
return an int as an error code.
*) Then change the call above to
if (ivtv_write_vbi(itv, (const __user struct v4l2_sliced_vbi_data *)user_buf, elems)<0) {
ivtv_release_stream(s);
return -EFAULT;
}
*) Then in ivtv_write_vbi I think the start of it's main loop could become something
like:
for (i = 0; i < cnt; i++) {
const struct v4l2_sliced_vbi_data d;
if (copy_from_user(&d, sliced+i, sizeof(struct v4l2_sliced_vbi_data))
return -EPERM;
then all the d-> to d.
and make sure it returns 0 in the other cases.
That leaves one bit I'm a bit more unsure about which is in ivtv_process_vbi_data there
is also a call to ivtv_write_vbi and I don't think that's using a __user pointer, so if
we change ivtv_write_vbi to take a __user pointer what happens? It doesn't seem to be
too unusual to pass kernel pointers to things that take __user pointers - but I don't
know enough about it to know whether that's the right thing to do (it'll generate a sparse
warning, but if it's actually secure unlike the current code it would be better I guess).
Dave
--
-----Open up your eyes, open up your mind, open up your code -------
/ Dr. David Alan Gilbert | Running GNU/Linux | Happy \
\ gro.gilbert @ treblig.org | | In Hex /
\ _________________________|_____ http://www.treblig.org |_______/
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
