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
New developer for RTL2832U DVB device
Good day, I would like to volunteer, to bring into the kernel, the support for Realtek DVB hardware. I need a word from Realtek about driver that is a direct port of windows driver. I need to know that if it is released under GPL compatible licence, which isn't clear today. Also I would like to access to newer revisions of this driver to keep up with the code, so that my future driver won't be out of date. And I need datasheets. Datasheets will save me a lot of time, time I would have to spend reverse engineering not obvious parts otherwise. For example, while reworking IR support to use ir-core, I run info few issues that are likely to be fixable using a different register setup. I don't ming agreeing to non written NDA, that is you gave me datasheets privately and ask me not to distribute them. I won't. Actually I have few datasheets already obtained that way. In this mail, I tried to CC few developers that have shown an interest in this driver. I also CCed few realtek employees whose addresses I could find in mail archive. I really do hope for an answer (don't mind if it is private). Best regards, Maxim Levitsky -- 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: RFC: Problem of using v4l2 spec with codec function
Hi all, I would side with Laurent on this. Judging by formats seems to be enough for this driver and it has great, in my opinion, advantages of a) not overcomplicating things for applications b) not adding new pieces to the API... -- Best regards, Pawel Osciak -- 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: [PATCH 7/7] v4l: videobuf2: add CMA allocator
Hi,
please see my comments below.
Also, if I could suggest something, please cut the code between
functions, not inside them, it'll make it easier for others to find
that location in the original code.
On Wed, Dec 1, 2010 at 00:56, Marek Szyprowski wrote:
> Hello,
>
> On Wednesday, December 01, 2010 9:36 AM Jonghun Han wrote:
>
>> Marek Szyprowski wrote:
>> 2010/11/20 Marek Szyprowski :
>> > From: Pawel Osciak
>> >
>> > Add support for the CMA contiguous memory allocator to videobuf2.
>> >
>> > Signed-off-by: Pawel Osciak
>> > Signed-off-by: Kyungmin Park
>> > Signed-off-by: Marek Szyprowski
>> > CC: Pawel Osciak
>> > ---
>>
>> Hi Marek,
>>
>>
>>
>> > +static void *vb2_cma_get_userptr(unsigned long vaddr, unsigned long size)
>>
>> static void *vb2_cma_get_userptr(unsigned long vaddr, unsigned long size,
>> const struct vb2_alloc_ctx
>> *alloc_ctx)
>> alloc_ctx can be useful for many cases.
>
> Yes, right, I will add it in the next version.
>
Actually, I don't think so. Can you give an example of one of those
"many cases"? There is no need to add unused arguments that "can be
useful in future" just for the sake of future extensions, especially
not in the kernel in such sensitive code. This function can
potentially be called on each frame. And even more importantly, the
whole allocator interface has been designed so that functions will not
have to be vb2- or media-specific. Ideally, we should be able to hook
up functions from mm and other modules. And such functions either use
only vaddr and size, or have means of finding their contexts
otherwise.
If I remember correctly, this approach was actually suggested by
Laurent and it makes a lot of sense. We want to be able to use third
party kernel functions in allocators as much as possbile.
>> > +{
>> > + struct vb2_cma_buf *buf;
>> > + unsigned long paddr = 0;
>> > + int ret;
>> > +
>> > + buf = kzalloc(sizeof *buf, GFP_KERNEL);
>> > + if (!buf)
>> > + return ERR_PTR(-ENOMEM);
>> > +
>> > + buf->vma = vb2_get_userptr(vaddr);
>>
>> vb2_get_vma looks good instead of vb2_get_userptr.
>> How do you think about this ?
>
> Right, this will make the code easier to understand. Thanks for the hint!
>
Actually, I don't think it's a good idea. I think you have the whole
concept backwards.
vb2 doesn't care what is returned from this function. All it wants is
a cookie from this allocator. From the point of view where this
function is used, it is "get_userptr" not "get_vma", vb2 wants to get
something describing a user pointer, but that does not have to mean a
vma. Moreover, in any case, the core does not care at all what it is
it is getting, since it *never uses it*. Changing the name of this
function would actually make it confusing.
In other words: this function is an implementation of
"get_userptr_private_allocator_cookie" allocator interface function
and *not* a "get_vma" function implementation. It does not have to be
a vma and the core does not care what it is anyway.
>> > + if (!buf->vma) {
>> > + printk(KERN_ERR "Failed acquiring VMA for vaddr
>> 0x%08lx\n",
>> > + vaddr);
>> > + ret = -EINVAL;
>> > + goto done;
>> > + }
>> > +
>> > + ret = vb2_contig_verify_userptr(buf->vma, vaddr, size, &paddr);
>> > + if (ret) {
>> > + vb2_put_userptr(buf->vma);
>> > + goto done;
>> > + }
>> > +
>>
>> In vb2_contig_verify_userptr, vma is re-found via find_vma although it has
>> been checked after vb2_get_userptr.
>> Why double checking is needed ?
>
> I'm not sure. I must check this core again, maybe it can be simplified a bit.
I agree, it seems that it should be enough to remove find_vma from the
gerneric verify contig function.
--
Best regards,
Pawel Osciak
--
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: [PATCH 09/10] MCDE: Add build files and bus
On Fri, Nov 26, 2010 at 6:24 AM, Arnd Bergmann wrote: > [dri people: please have a look at the KMS discussion way below] > > On Thursday 25 November 2010 19:00:26 Marcus LORENTZON wrote: >> > -Original Message- >> > From: Arnd Bergmann [mailto:a...@arndb.de] >> > Sent: den 25 november 2010 17:48 >> > To: Marcus LORENTZON >> > Cc: linux-arm-ker...@lists.infradead.org; Jimmy RUBIN; linux- >> > me...@vger.kernel.org; Dan JOHANSSON; Linus WALLEIJ >> > Subject: Re: [PATCH 09/10] MCDE: Add build files and bus >> > >> > On Thursday 25 November 2010, Marcus LORENTZON wrote: >> > > From: Arnd Bergmann [mailto:a...@arndb.de] >> > > > On Wednesday 10 November 2010, Jimmy Rubin wrote: >> > > > > This patch adds support for the MCDE, Memory-to-display >> > controller, >> > > > > found in the ST-Ericsson ux500 products. >> > > > > > > [note: please configure your email client properly so it keeps > proper attribution of text and and does not rewrap the citations > incorrectly. Wrap your own text after 70 characters] > >> > >> > All devices that you cannot probe by asking hardware or firmware are >> > normally >> > considered platform devices. Then again, a platform device is usally >> > identified by its resources, i.e. MMIO addresses and interrupts, which >> > I guess your display does not have. >> >> Then we might be on right track to model them as devices on a >> platform bus. Since most displays/panels can't be "plug-n-play" >> probed, instead devices has to be statically declared in >> board-xx.c files in mach-ux500 folder. Or is platform bus a >> "singleton"? Or can we define a new platform bus device? >> Displays like HDMI TV-sets are not considered for device/driver >> in mcde. Instead there will be a hdmi-chip-device/driver on the >> mcde bus. So all devices and drivers on this bus are static. > > I think I need to clarify to things: > > * When I talk about a bus, I mean 'struct bus_type', which identifies > all devices with a uniform bus interface to their parent device > (think: PCI, USB, I2C). You seem to think of a bus as a specific > instance of that bus type, i.e. the device that is the parent of > all the connected devices. If you have only one instance of a bus > in any system, and they are all using the same driver, do not add > a bus_type for it. > A good reason to add a bus_type would be e.g. if the "display" > driver uses interfaces to the dss that are common among multiple > dss drivers from different vendors, but the actual display drivers > are identical. This does not seem to be the case. > > * When you say that the devices are static, I hope you do not mean > static in the C language sense. We used to allow devices to be > declared as "static struct" and registered using > platform_device_register (or other bus specific functions). This > is no longer valid and we are removing the existing users, do not > add new ones. When creating a platform device, use > platform_device_register_simple or platform_device_register_resndata. > > I'm not sure what you mean with drivers being static. Predefining > the association between displays and drivers in per-machine files is > fine, but since this is really board specific, it would be better > to eventually do this through data passed from the boot loader, so > you don't have to have a machine file for every combination of displays > that is in the field. > >> > Staging it in a way that adds all the display drivers later than the >> > base driver is a good idea, but it would be helpful to also add the >> > infrastructure at the later stage. Maybe you can try to simplify the >> > code for now by hardcoding the single display and remove the dynamic >> > registration. You still have the the code, so once the base code looks >> > good for inclusion, we can talk about it in the context of adding >> > dynamic display support back in, possibly in exactly the way you are >> > proposing now, but perhaps in an entirely different way if we come up >> > with a better solution. >> >> What about starting with MCDE HW, which is the core HW driver doing >> all real work? And then continue with the infrastructure + some displays >> + drivers ... > > This is already the order in which you submitted them, I don't see a > difference here. I was not asking to delay any of the code, just to put > them in a logical order. > >> Only problem is that we then have a driver that can't be used from user >> space, because I don't think I can find anyone with enough time to write >> a display driver + framebuffer on top of mcde_hw (which is what the >> existing code does). > > Well, developer time does not appear to be one of your problems, you > already wasted tons of it by developing a huge chunk of code that isn't > going anywhere because you wrote it without consulting the upstream > community ;-) > > There is no need to develop anything from scratch here, you already have > the code you want to end up with. What I would do here is to start with > a single
[PATCH v2] media: rc: ir-lirc-codec: fix integer overflow
'n' may be bigger than MAX_INT*sizeof(int), if so checking of truncated (int)(n/sizeof(int)) for LIRCBUF_SIZE overflows and then using nontruncated 'count' doesn't make sense. This is not a security issue as too big 'n' is catched in kmalloc() in memdup_user() call. However, it's better to prevent WARN() in kmalloc(). Signed-off-by: Vasiliy Kulikov --- Compile tested only. drivers/media/rc/ir-lirc-codec.c |3 ++- 1 files changed, 2 insertions(+), 1 deletions(-) diff --git a/drivers/media/rc/ir-lirc-codec.c b/drivers/media/rc/ir-lirc-codec.c index 1e87ee8..a7e91e6 100644 --- a/drivers/media/rc/ir-lirc-codec.c +++ b/drivers/media/rc/ir-lirc-codec.c @@ -100,7 +100,8 @@ static ssize_t ir_lirc_transmit_ir(struct file *file, const char *buf, struct lirc_codec *lirc; struct rc_dev *dev; int *txbuf; /* buffer with values to transmit */ - int ret = 0, count; + int ret = 0; + size_t count; lirc = lirc_get_pdata(file); if (!lirc) -- 1.7.0.4 -- 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
[cron job] v4l-dvb daily build: WARNINGS
This message is generated daily by a cron job that builds v4l-dvb for the kernels and architectures in the list below. Results of the daily build of v4l-dvb: date:Sat Dec 4 19:00:14 CET 2010 git master: 59365d136d205cc20fe666ca7f89b1c5001b0d5a git media-master: gcc version: i686-linux-gcc (GCC) 4.5.1 host hardware:x86_64 host os: 2.6.32.5 linux-git-armv5: WARNINGS linux-git-armv5-davinci: WARNINGS linux-git-armv5-ixp: WARNINGS linux-git-armv5-omap2: WARNINGS linux-git-i686: WARNINGS linux-git-m32r: WARNINGS linux-git-mips: WARNINGS linux-git-powerpc64: WARNINGS linux-git-x86_64: WARNINGS spec-git: OK sparse: ERRORS Detailed results are available here: http://www.xs4all.nl/~hverkuil/logs/Saturday.log Full logs are available here: http://www.xs4all.nl/~hverkuil/logs/Saturday.tar.bz2 The V4L-DVB specification from this daily build is here: http://www.xs4all.nl/~hverkuil/spec/media.html -- 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
[PATCH] DVB/V4L: Fix Kconfig select/depend conflicts
The selection of 'helper' drivers for peripheral chips can be done either automatically by the config entry for the controller chip in a device (default if !EMBEDDED) or manually if the user knows exactly which peripheral chips are used. The config entries for these helper drivers are completely hidden if automatic selection is enabled. However, the way that this is currently done results in a config dependency on manual selection, so Kconfig now warns of symbols being auto-selected without their dependencies being met. This changes all those config entries so that only their visibility depends on manual selection being enabled. It also necessarily removes some explicit menus, since explicit menus always result in a config dependency. Signed-off-by: Ben Hutchings --- Mauro, please try to get this included in 2.6.37. Now that Kconfig warns about such conflicts, it results in an enormous amount of noise in an all-modules config. Ben. drivers/media/common/tuners/Kconfig | 61 ++ drivers/media/dvb/frontends/Kconfig | 245 ++- drivers/media/video/Kconfig | 138 +--- drivers/media/video/cx25840/Kconfig |3 +- 4 files changed, 287 insertions(+), 160 deletions(-) diff --git a/drivers/media/common/tuners/Kconfig b/drivers/media/common/tuners/Kconfig index 2385e6c..adac6cf 100644 --- a/drivers/media/common/tuners/Kconfig +++ b/drivers/media/common/tuners/Kconfig @@ -44,10 +44,9 @@ menuconfig MEDIA_TUNER_CUSTOMISE If unsure say N. -if MEDIA_TUNER_CUSTOMISE - config MEDIA_TUNER_SIMPLE - tristate "Simple tuner support" + tristate + prompt "Simple tuner support" if MEDIA_TUNER_CUSTOMISE depends on VIDEO_MEDIA && I2C select MEDIA_TUNER_TDA9887 default m if MEDIA_TUNER_CUSTOMISE @@ -55,7 +54,8 @@ config MEDIA_TUNER_SIMPLE Say Y here to include support for various simple tuners. config MEDIA_TUNER_TDA8290 - tristate "TDA 8290/8295 + 8275(a)/18271 tuner combo" + tristate + prompt "TDA 8290/8295 + 8275(a)/18271 tuner combo" if MEDIA_TUNER_CUSTOMISE depends on VIDEO_MEDIA && I2C select MEDIA_TUNER_TDA827X select MEDIA_TUNER_TDA18271 @@ -64,21 +64,24 @@ config MEDIA_TUNER_TDA8290 Say Y here to include support for Philips TDA8290+8275(a) tuner. config MEDIA_TUNER_TDA827X - tristate "Philips TDA827X silicon tuner" + tristate + prompt "Philips TDA827X silicon tuner" if MEDIA_TUNER_CUSTOMISE depends on VIDEO_MEDIA && I2C default m if MEDIA_TUNER_CUSTOMISE help A DVB-T silicon tuner module. Say Y when you want to support this tuner. config MEDIA_TUNER_TDA18271 - tristate "NXP TDA18271 silicon tuner" + tristate + prompt "NXP TDA18271 silicon tuner" if MEDIA_TUNER_CUSTOMISE depends on VIDEO_MEDIA && I2C default m if MEDIA_TUNER_CUSTOMISE help A silicon tuner module. Say Y when you want to support this tuner. config MEDIA_TUNER_TDA9887 - tristate "TDA 9885/6/7 analog IF demodulator" + tristate + prompt "TDA 9885/6/7 analog IF demodulator" if MEDIA_TUNER_CUSTOMISE depends on VIDEO_MEDIA && I2C default m if MEDIA_TUNER_CUSTOMISE help @@ -86,7 +89,8 @@ config MEDIA_TUNER_TDA9887 analog IF demodulator. config MEDIA_TUNER_TEA5761 - tristate "TEA 5761 radio tuner (EXPERIMENTAL)" + tristate + prompt "TEA 5761 radio tuner (EXPERIMENTAL)" if MEDIA_TUNER_CUSTOMISE depends on VIDEO_MEDIA && I2C depends on EXPERIMENTAL default m if MEDIA_TUNER_CUSTOMISE @@ -94,56 +98,64 @@ config MEDIA_TUNER_TEA5761 Say Y here to include support for the Philips TEA5761 radio tuner. config MEDIA_TUNER_TEA5767 - tristate "TEA 5767 radio tuner" + tristate + prompt "TEA 5767 radio tuner" if MEDIA_TUNER_CUSTOMISE depends on VIDEO_MEDIA && I2C default m if MEDIA_TUNER_CUSTOMISE help Say Y here to include support for the Philips TEA5767 radio tuner. config MEDIA_TUNER_MT20XX - tristate "Microtune 2032 / 2050 tuners" + tristate + prompt "Microtune 2032 / 2050 tuners" if MEDIA_TUNER_CUSTOMISE depends on VIDEO_MEDIA && I2C default m if MEDIA_TUNER_CUSTOMISE help Say Y here to include support for the MT2032 / MT2050 tuner. config MEDIA_TUNER_MT2060 - tristate "Microtune MT2060 silicon IF tuner" + tristate + prompt "Microtune MT2060 silicon IF tuner" if MEDIA_TUNER_CUSTOMISE depends on VIDEO_MEDIA && I2C default m if MEDIA_TUNER_CUSTOMISE help A driver for the silicon IF tuner MT2060 from Microtune. config MEDIA_TUNER_MT2266 - tristate "Microtune MT2266 silicon tuner" + tristate + prompt "Microtune MT2266 silicon tuner" if MEDIA_TUNER_CUSTOMISE depends on VIDEO_MED
