Mailing List Archive

user accesses in ivtv-fileops.c:ivtv_v4l2_write ?
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.

Is there something that makes this safe?

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 |_______/

_______________________________________________
ivtv-devel mailing list
ivtv-devel@ivtvdriver.org
http://ivtvdriver.org/mailman/listinfo/ivtv-devel
Re: user accesses in ivtv-fileops.c:ivtv_v4l2_write ? [ In reply to ]
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.

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.

>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:

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. :)

Regards,
Andy



_______________________________________________
ivtv-devel mailing list
ivtv-devel@ivtvdriver.org
http://ivtvdriver.org/mailman/listinfo/ivtv-devel
Re: user accesses in ivtv-fileops.c:ivtv_v4l2_write ? [ In reply to ]
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 |_______/

_______________________________________________
ivtv-devel mailing list
ivtv-devel@ivtvdriver.org
http://ivtvdriver.org/mailman/listinfo/ivtv-devel
Re: user accesses in ivtv-fileops.c:ivtv_v4l2_write ? [ In reply to ]
On Sun, 2010-11-28 at 17:40 +0000, 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);
>

Hi David,

Let me know if this patch works for your sparse build and adequately
addresses the problem.

It might be easiest to review this patch by starting at the bottom and
working your way up.

Regards,
Andy


ivtv: ivtv_write_vbi() should use copy_from_user() for user data buffers

ivtv_write_vbi() is used to output VBI data to a TV screen using the
CX23415's decoder. It was used for both VBI data that came from the
driver internally and VBI data that came from the user, but did not use
copy_from_user() for reading the VBI data from the user buffers.

This change adds a new version of the function,
ivtv_write_vbi_from_user(), that uses copy_from_user() to read the VBI
data provided via user buffers.

This should resolve an sparse build warning reported by Dave Gilbert.

Reported-by: Dr. David Alan Gilbert <linux@treblig.org>
Signed-off-by: Andy Walls <awalls@md.metrocast.net>


diff --git a/drivers/media/video/ivtv/ivtv-fileops.c b/drivers/media/video/ivtv/ivtv-fileops.c
index d727485..4f46b00 100644
--- a/drivers/media/video/ivtv/ivtv-fileops.c
+++ b/drivers/media/video/ivtv/ivtv-fileops.c
@@ -570,7 +570,8 @@ ssize_t ivtv_v4l2_write(struct file *filp, const char __user *user_buf, size_t c
int elems = count / sizeof(struct v4l2_sliced_vbi_data);

set_bit(IVTV_F_S_APPL_IO, &s->s_flags);
- ivtv_write_vbi(itv, (const struct v4l2_sliced_vbi_data *)user_buf, elems);
+ ivtv_write_vbi_from_user(itv,
+ (const struct v4l2_sliced_vbi_data __user *)user_buf, elems);
return elems * sizeof(struct v4l2_sliced_vbi_data);
}

diff --git a/drivers/media/video/ivtv/ivtv-vbi.c b/drivers/media/video/ivtv/ivtv-vbi.c
index e1c347e..7275f2d 100644
--- a/drivers/media/video/ivtv/ivtv-vbi.c
+++ b/drivers/media/video/ivtv/ivtv-vbi.c
@@ -92,54 +92,91 @@ static int odd_parity(u8 c)
return c & 1;
}

-void ivtv_write_vbi(struct ivtv *itv, const struct v4l2_sliced_vbi_data *sliced, size_t cnt)
+static void ivtv_write_vbi_line(struct ivtv *itv,
+ const struct v4l2_sliced_vbi_data *d,
+ struct vbi_cc *cc, int *found_cc)
{
struct vbi_info *vi = &itv->vbi;
- struct vbi_cc cc = { .odd = { 0x80, 0x80 }, .even = { 0x80, 0x80 } };
- int found_cc = 0;
- size_t i;
-
- for (i = 0; i < cnt; i++) {
- const struct v4l2_sliced_vbi_data *d = sliced + i;

- if (d->id == V4L2_SLICED_CAPTION_525 && d->line == 21) {
- if (d->field) {
- cc.even[0] = d->data[0];
- cc.even[1] = d->data[1];
- } else {
- cc.odd[0] = d->data[0];
- cc.odd[1] = d->data[1];
- }
- found_cc = 1;
+ if (d->id == V4L2_SLICED_CAPTION_525 && d->line == 21) {
+ if (d->field) {
+ cc->even[0] = d->data[0];
+ cc->even[1] = d->data[1];
+ } else {
+ cc->odd[0] = d->data[0];
+ cc->odd[1] = d->data[1];
}
- else if (d->id == V4L2_SLICED_VPS && d->line == 16 && d->field == 0) {
- struct vbi_vps vps;
-
- vps.data[0] = d->data[2];
- vps.data[1] = d->data[8];
- vps.data[2] = d->data[9];
- vps.data[3] = d->data[10];
- vps.data[4] = d->data[11];
- if (memcmp(&vps, &vi->vps_payload, sizeof(vps))) {
- vi->vps_payload = vps;
- set_bit(IVTV_F_I_UPDATE_VPS, &itv->i_flags);
- }
+ *found_cc = 1;
+ } else if (d->id == V4L2_SLICED_VPS && d->line == 16 && d->field == 0) {
+ struct vbi_vps vps;
+
+ vps.data[0] = d->data[2];
+ vps.data[1] = d->data[8];
+ vps.data[2] = d->data[9];
+ vps.data[3] = d->data[10];
+ vps.data[4] = d->data[11];
+ if (memcmp(&vps, &vi->vps_payload, sizeof(vps))) {
+ vi->vps_payload = vps;
+ set_bit(IVTV_F_I_UPDATE_VPS, &itv->i_flags);
}
- else if (d->id == V4L2_SLICED_WSS_625 && d->line == 23 && d->field == 0) {
- int wss = d->data[0] | d->data[1] << 8;
+ } else if (d->id == V4L2_SLICED_WSS_625 &&
+ d->line == 23 && d->field == 0) {
+ int wss = d->data[0] | d->data[1] << 8;

- if (vi->wss_payload != wss) {
- vi->wss_payload = wss;
- set_bit(IVTV_F_I_UPDATE_WSS, &itv->i_flags);
- }
+ if (vi->wss_payload != wss) {
+ vi->wss_payload = wss;
+ set_bit(IVTV_F_I_UPDATE_WSS, &itv->i_flags);
}
}
- if (found_cc && vi->cc_payload_idx < ARRAY_SIZE(vi->cc_payload)) {
- vi->cc_payload[vi->cc_payload_idx++] = cc;
+}
+
+static void ivtv_write_vbi_cc_lines(struct ivtv *itv, const struct vbi_cc *cc)
+{
+ struct vbi_info *vi = &itv->vbi;
+
+ if (vi->cc_payload_idx < ARRAY_SIZE(vi->cc_payload)) {
+ memcpy(&vi->cc_payload[vi->cc_payload_idx], cc,
+ sizeof(struct vbi_cc));
+ vi->cc_payload_idx++;
set_bit(IVTV_F_I_UPDATE_CC, &itv->i_flags);
}
}

+static void ivtv_write_vbi(struct ivtv *itv,
+ const struct v4l2_sliced_vbi_data *sliced,
+ size_t cnt)
+{
+ struct vbi_cc cc = { .odd = { 0x80, 0x80 }, .even = { 0x80, 0x80 } };
+ int found_cc = 0;
+ size_t i;
+
+ for (i = 0; i < cnt; i++)
+ ivtv_write_vbi_line(itv, sliced + i, &cc, &found_cc);
+
+ if (found_cc)
+ ivtv_write_vbi_cc_lines(itv, &cc);
+}
+
+void ivtv_write_vbi_from_user(struct ivtv *itv,
+ const struct v4l2_sliced_vbi_data __user *sliced,
+ size_t cnt)
+{
+ struct vbi_cc cc = { .odd = { 0x80, 0x80 }, .even = { 0x80, 0x80 } };
+ int found_cc = 0;
+ size_t i;
+ struct v4l2_sliced_vbi_data d;
+
+ for (i = 0; i < cnt; i++) {
+ if (copy_from_user(&d, sliced + i,
+ sizeof(struct v4l2_sliced_vbi_data)))
+ break;
+ ivtv_write_vbi_line(itv, sliced + i, &cc, &found_cc);
+ }
+
+ if (found_cc)
+ ivtv_write_vbi_cc_lines(itv, &cc);
+}
+
static void copy_vbi_data(struct ivtv *itv, int lines, u32 pts_stamp)
{
int line = 0;
diff --git a/drivers/media/video/ivtv/ivtv-vbi.h b/drivers/media/video/ivtv/ivtv-vbi.h
index 970567b..eda38d0 100644
--- a/drivers/media/video/ivtv/ivtv-vbi.h
+++ b/drivers/media/video/ivtv/ivtv-vbi.h
@@ -20,7 +20,9 @@
#ifndef IVTV_VBI_H
#define IVTV_VBI_H

-void ivtv_write_vbi(struct ivtv *itv, const struct v4l2_sliced_vbi_data *sliced, size_t count);
+void ivtv_write_vbi_from_user(struct ivtv *itv,
+ const struct v4l2_sliced_vbi_data __user *sliced,
+ size_t count);
void ivtv_process_vbi_data(struct ivtv *itv, struct ivtv_buffer *buf,
u64 pts_stamp, int streamtype);
int ivtv_used_line(struct ivtv *itv, int line, int field);



_______________________________________________
ivtv-devel mailing list
ivtv-devel@ivtvdriver.org
http://ivtvdriver.org/mailman/listinfo/ivtv-devel
Re: user accesses in ivtv-fileops.c:ivtv_v4l2_write ? [ In reply to ]
* Andy Walls (awalls@md.metrocast.net) wrote:
> On Sun, 2010-11-28 at 17:40 +0000, 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);
> >
>
> Hi David,
>
> Let me know if this patch works for your sparse build and adequately
> addresses the problem.

Hi Andy,
Yes that seems to fix it.
The only other comment I have is that it would probably be better if
ivtv_write_vbi_from_user() were to return an error if the copy_from_user
were to fail and then pass that all the way back up so that if an app passed
a bad pointer in it would get an EFAULT or the like.

Thanks,

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 |_______/

_______________________________________________
ivtv-devel mailing list
ivtv-devel@ivtvdriver.org
http://ivtvdriver.org/mailman/listinfo/ivtv-devel
Re: user accesses in ivtv-fileops.c:ivtv_v4l2_write ? [ In reply to ]
On Sun, 2011-01-09 at 00:34 +0000, Dr. David Alan Gilbert wrote:
> Hi Andy,
> It looks like we missed something in that copy from user
> patch from the end of last year:
>
> +void ivtv_write_vbi_from_user(struct ivtv *itv,
> + const struct v4l2_sliced_vbi_data __user *sliced,
> + size_t cnt)
> +{
> + struct vbi_cc cc = { .odd = { 0x80, 0x80 }, .even = { 0x80, 0x80 } };
> + int found_cc = 0;
> + size_t i;
> + struct v4l2_sliced_vbi_data d;
> +
> + for (i = 0; i < cnt; i++) {
> + if (copy_from_user(&d, sliced + i,
> + sizeof(struct v4l2_sliced_vbi_data)))
> + break;
> + ivtv_write_vbi_line(itv, sliced + i, &cc, &found_cc);
^^^^^^^^^^
What was I thinking? ---------------------+

Decent plan; failed execution. :(


>
> sparse is giving me:
> drivers/media/video/ivtv/ivtv-vbi.c:177:49: warning: incorrect type in argument 2 (different address spaces)
> drivers/media/video/ivtv/ivtv-vbi.c:177:49: expected struct v4l2_sliced_vbi_data const *d
> drivers/media/video/ivtv/ivtv-vbi.c:177:49: got struct v4l2_sliced_vbi_data const [noderef] <asn:1>*
>
> and I think the point is that while you've copied the data I think
> you're still passing the user pointer to ivtv_write_vbi_line and it
> should be:
>
> ivtv_write_vbi_line(itv, &d, &cc, &found_cc);
>
>
> What do you think?

Yes, it looks like I gooned that one up. :)

That's what I get for trying to fix things with the kids running around
before bedtime.

I assume that you have made the replacement and tested that sparse is
satisfied?

Regards,
Andy


_______________________________________________
ivtv-devel mailing list
ivtv-devel@ivtvdriver.org
http://ivtvdriver.org/mailman/listinfo/ivtv-devel
Re: user accesses in ivtv-fileops.c:ivtv_v4l2_write ? [ In reply to ]
Hi Andy,
It looks like we missed something in that copy from user
patch from the end of last year:

+void ivtv_write_vbi_from_user(struct ivtv *itv,
+ const struct v4l2_sliced_vbi_data __user *sliced,
+ size_t cnt)
+{
+ struct vbi_cc cc = { .odd = { 0x80, 0x80 }, .even = { 0x80, 0x80 } };
+ int found_cc = 0;
+ size_t i;
+ struct v4l2_sliced_vbi_data d;
+
+ for (i = 0; i < cnt; i++) {
+ if (copy_from_user(&d, sliced + i,
+ sizeof(struct v4l2_sliced_vbi_data)))
+ break;
+ ivtv_write_vbi_line(itv, sliced + i, &cc, &found_cc);


sparse is giving me:
drivers/media/video/ivtv/ivtv-vbi.c:177:49: warning: incorrect type in argument 2 (different address spaces)
drivers/media/video/ivtv/ivtv-vbi.c:177:49: expected struct v4l2_sliced_vbi_data const *d
drivers/media/video/ivtv/ivtv-vbi.c:177:49: got struct v4l2_sliced_vbi_data const [noderef] <asn:1>*

and I think the point is that while you've copied the data I think
you're still passing the user pointer to ivtv_write_vbi_line and it
should be:

ivtv_write_vbi_line(itv, &d, &cc, &found_cc);


What do you think?

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 |_______/

_______________________________________________
ivtv-devel mailing list
ivtv-devel@ivtvdriver.org
http://ivtvdriver.org/mailman/listinfo/ivtv-devel
Re: user accesses in ivtv-fileops.c:ivtv_v4l2_write ? [ In reply to ]
* Andy Walls (awalls@md.metrocast.net) wrote:
> On Sun, 2011-01-09 at 00:34 +0000, Dr. David Alan Gilbert wrote:
> > Hi Andy,
> > It looks like we missed something in that copy from user
> > patch from the end of last year:
> >
> > +void ivtv_write_vbi_from_user(struct ivtv *itv,
> > + const struct v4l2_sliced_vbi_data __user *sliced,
> > + size_t cnt)
> > +{
> > + struct vbi_cc cc = { .odd = { 0x80, 0x80 }, .even = { 0x80, 0x80 } };
> > + int found_cc = 0;
> > + size_t i;
> > + struct v4l2_sliced_vbi_data d;
> > +
> > + for (i = 0; i < cnt; i++) {
> > + if (copy_from_user(&d, sliced + i,
> > + sizeof(struct v4l2_sliced_vbi_data)))
> > + break;
> > + ivtv_write_vbi_line(itv, sliced + i, &cc, &found_cc);
> ^^^^^^^^^^
> What was I thinking? ---------------------+
>
> Decent plan; failed execution. :(

Sorry for not spotting it when you sent it out last time!

> > sparse is giving me:
> > drivers/media/video/ivtv/ivtv-vbi.c:177:49: warning: incorrect type in argument 2 (different address spaces)
> > drivers/media/video/ivtv/ivtv-vbi.c:177:49: expected struct v4l2_sliced_vbi_data const *d
> > drivers/media/video/ivtv/ivtv-vbi.c:177:49: got struct v4l2_sliced_vbi_data const [noderef] <asn:1>*
> >
> > and I think the point is that while you've copied the data I think
> > you're still passing the user pointer to ivtv_write_vbi_line and it
> > should be:
> >
> > ivtv_write_vbi_line(itv, &d, &cc, &found_cc);
> >
> >
> > What do you think?
>
> Yes, it looks like I gooned that one up. :)
>
> That's what I get for trying to fix things with the kids running around
> before bedtime.
>
> I assume that you have made the replacement and tested that sparse is
> satisfied?

Yes, seems happy - of course I thought I did last time :-)

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 |_______/

_______________________________________________
ivtv-devel mailing list
ivtv-devel@ivtvdriver.org
http://ivtvdriver.org/mailman/listinfo/ivtv-devel