[PATCH] Guard a divide in v4l1 compat layer
Hi,
I managed to trigger a divide by 0 in the v4l compat code
with the mem2mem test module; I suspect perhaps it shouldn't
have been returning a 0 pixel wide picture, but either way it seems
right to guard this divide by 0 in the compatibility layer.
Tested on 2.6.36 (ubuntu build, but the code in this is the same as upstream),
but ***not tested with a real video device***.
Signed-off-by: Dr. David Alan Gilbert li...@treblig.org
--
diff --git a/drivers/media/video/v4l1-compat.c
b/drivers/media/video/v4l1-compat.c
index 0c2105c..d4ac751 100644
--- a/drivers/media/video/v4l1-compat.c
+++ b/drivers/media/video/v4l1-compat.c
@@ -645,9 +645,16 @@ static noinline long v4l1_compat_get_picture(
goto done;
}
- pict-depth = ((fmt-fmt.pix.bytesperline 3)
-+ (fmt-fmt.pix.width - 1))
-/ fmt-fmt.pix.width;
+ if (fmt-fmt.pix.width)
+ {
+ pict-depth = ((fmt-fmt.pix.bytesperline 3)
++ (fmt-fmt.pix.width - 1))
+/ fmt-fmt.pix.width;
+ } else {
+ err = -EINVAL;
+ goto done;
+ }
+
pict-palette = pixelformat_to_palette(
fmt-fmt.pix.pixelformat);
done:
--
-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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
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 |___/ -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: user accesses in ivtv-fileops.c:ivtv_v4l2_write ?
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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: user accesses in ivtv-fileops.c:ivtv_v4l2_write ?
* Andy Walls (awa...@md.metrocast.net) wrote: On Sun, 2010-11-28 at 17:40 +, 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 |___/ -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: user accesses in ivtv-fileops.c:ivtv_v4l2_write ?
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 |___/
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: user accesses in ivtv-fileops.c:ivtv_v4l2_write ?
* Andy Walls (awa...@md.metrocast.net) wrote:
On Sun, 2011-01-09 at 00:34 +, 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 |___/
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Typo in dib0700_devices.c
Hi, sparse lead me to what looks like a typo in dib0700_devices.c: drivers/media/dvb/dvb-usb/dib0700_devices.c:2160:18: warning: Initializer entry defined twice drivers/media/dvb/dvb-usb/dib0700_devices.c:2165:18: also defined here 2160.agc1_pt1 = 0, .agc1_pt2 = 0, .agc1_pt3 = 98, .agc1_slope1= 0, .agc1_slope2= 167, 2165.agc1_pt1 = 98, .agc2_pt2 = 255, Note the line 2165 says .agc1_pt1 - I believe that should be agc*2*_pt1 (without knowing anything at all about your hardware!) The last thing that changed that was Olivier's patch b4d6046e841955be9cc49164b03b91c9524f9c2e which was just a tidy up, but it had: - .agc1_pt1 = 98, // agc2_pt1 + .agc1_pt1 = 98, Note the mismatch of the comment. (I haven't got this hardware to test on - but if you've been having some weird AGC issues on that card it might explain it!) 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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
vb2_plane 'mapped' signed bit field
Hi Pawel,
'sparse' spotted that vb2_plane's mapped field is a signed
bitfield:
include/media/videobuf2-core.h:78:41 1 bit signed int
struct vb2_plane {
void*mem_priv;
int mapped:1;
};
that probably should be an unsigned int (I can see code that assigns
1 to it that just won't fit).
(Introduced by e23ccc0ad9258634e6d52cedf473b35dc34416c7 , spotted in
2.6.39-rc1 )
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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: vb2_plane 'mapped' signed bit field
(Originally sent to Pawel's Samsung address that bounced)
* Dr. David Alan Gilbert (li...@treblig.org) wrote:
Hi Pawel,
'sparse' spotted that vb2_plane's mapped field is a signed
bitfield:
include/media/videobuf2-core.h:78:41 1 bit signed int
struct vb2_plane {
void*mem_priv;
int mapped:1;
};
that probably should be an unsigned int (I can see code that assigns
1 to it that just won't fit).
(Introduced by e23ccc0ad9258634e6d52cedf473b35dc34416c7 , spotted in
2.6.39-rc1 )
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 |___/
--
-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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
