diff mbox

[V2] drivers/media/v4l2-core/videobuf2-core.c: fix error return code

Message ID 1350571624-4666-1-git-send-email-peter.senna@gmail.com (mailing list archive)
State New, archived
Headers show

Commit Message

Peter Senna Tschudin Oct. 18, 2012, 2:47 p.m. UTC
This patch fixes a NULL pointer dereference bug at __vb2_init_fileio().
The NULL pointer deference happens at videobuf2-core.c:

static size_t __vb2_perform_fileio(struct vb2_queue *q, char __user *data, size_t count,
                loff_t *ppos, int nonblock, int read)
{
...
        if (!q->fileio) {
                ret = __vb2_init_fileio(q, read);
                dprintk(3, "file io: vb2_init_fileio result: %d\n", ret);
                if (ret)
                        return ret;
        }
        fileio = q->fileio; // NULL pointer deference here
...
}

It was tested with vivi driver and qv4l2 for selecting read() as capture method.
The OOPS happened when I've artificially forced the error by commenting the line:
	if (fileio->bufs[i].vaddr == NULL)

But it was fixed after applying this patch.

[ 9002.451154] BUG: unable to handle kernel NULL pointer dereference at 0000000000000370
[ 9002.451266] IP: [<ffffffffa038c429>] __vb2_perform_fileio+0x69/0x620 [videobuf2_core]
[ 9002.451365] PGD 1df491067 PUD 196d12067 PMD 0
[ 9002.451431] Oops: 0000 [#1] SMP
[ 9002.451481] Modules linked in: vivi(O) v4l2_common videobuf2_core(O) videobuf2_vmalloc(O) videobuf2_memops(O) videodev hidp fuse ebtable_nat ebtables snd_hda_codec_hdmi snd_hda_codec_realtek ipt_MASQUERADE iptable_nat nf_nat xt_CHECKSUM iptable_mangle lockd sunrpc bridge stp llc rfcomm bnep btusb bluetooth pl2303 ip6t_REJECT snd_usb_audio nf_conntrack_ipv6 nf_defrag_ipv6 snd_usbmidi_lib snd_rawmidi ip6table_filter nf_conntrack_ipv4 ip6_tables nf_defrag_ipv4 xt_state nf_conntrack be2iscsi iscsi_boot_sysfs bnx2i cnic uio cxgb4i cxgb4 cxgb3i cxgb3 mdio libcxgbi ib_iser rdma_cm ib_addr iw_cm ib_cm ib_sa ib_mad ib_core iscsi_tcp libiscsi_tcp \
libiscsi scsi_transport_iscsi iTCO_wdt iTCO_vendor_support arc4 binfmt_misc media iwldvm mac80211 coretemp snd_hda_intel snd_hda_codec crc32c_intel snd_hwdep ghash_clmulni_intel snd_seq snd_seq_device microcode snd_pcm cdc_ncm usbnet mii cdc_wdm cdc_acm i915 snd_page_alloc i2c_algo_bit drm_kms_helper iwlwifi lpc_ich snd_timer sdhci_pci mfd_core drm toshiba_acpi sdhci snd sparse_keymap e1000e mmc_core cfg80211 i2c_core soundcore mei rfkill wmi video toshiba_bluetooth vhost_net tun macvtap macvlan kvm_intel kvm uinput [last unloaded: videobuf2_memops]
[ 9002.453105] CPU 1
[ 9002.453136] Pid: 13830, comm: qv4l2 Tainted: G           O 3.6.1-1.local.fc17.x86_64 #1 TOSHIBA PORTEGE R830/Portable PC
[ 9002.453251] RIP: 0010:[<ffffffffa038c429>]  [<ffffffffa038c429>] __vb2_perform_fileio+0x69/0x620 [videobuf2_core]
[ 9002.453368] RSP: 0018:ffff8801ed37fdf8  EFLAGS: 00010246
[ 9002.453427] RAX: 0000000000000000 RBX: 0000000000000000 RCX: 0000000000011b55
[ 9002.453502] RDX: 0000000000011b54 RSI: ffff88024e256f68 RDI: ffff88023fdc9c00
[ 9002.453576] RBP: ffff8801ed37fe58 R08: 0000000000016aa0 R09: 0000000000000001
[ 9002.453651] R10: 0000000000000075 R11: 0000000000000800 R12: ffff8801bcde0580
[ 9002.453726] R13: ffff8801bcde0608 R14: 00000000000e1000 R15: 00000000024f0ee0
[ 9002.453801] FS:  00007f19308f17c0(0000) GS:ffff88024e240000(0000) knlGS:0000000000000000
[ 9002.453886] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 9002.453948] CR2: 0000000000000370 CR3: 00000001df7df000 CR4: 00000000000407e0
[ 9002.454023] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
[ 9002.454098] DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000400
[ 9002.454175] Process qv4l2 (pid: 13830, threadinfo ffff8801ed37e000, task ffff8801f24b9710)
[ 9002.454260] Stack:
[ 9002.454284]  0000000000000000 0000000000000000 0000000000000800 0000000000000001
[ 9002.454379]  ffff8801ed37ff50 0000000000000000 ffff88023d56c610 ffff8801bcde0108
[ 9002.454473]  ffff8801bcde0580 00000000000e1000 ffff8801ed37ff50 ffff88023d56c600
[ 9002.454566] Call Trace:
[ 9002.454604]  [<ffffffffa038cb54>] vb2_read+0x14/0x20 [videobuf2_core]
[ 9002.454676]  [<ffffffffa038cc07>] vb2_fop_read+0xa7/0x4a0 [videobuf2_core]
[ 9002.454757]  [<ffffffffa03dc5e0>] v4l2_read+0xf0/0x140 [videodev]
[ 9002.454853]  [<ffffffff8118e911>] ? rw_verify_area+0x61/0xf0
[ 9002.454918]  [<ffffffff8118eda9>] vfs_read+0xa9/0x180
[ 9002.454976]  [<ffffffff8118eeca>] sys_read+0x4a/0x90
[ 9002.455035]  [<ffffffff816226e9>] system_call_fastpath+0x16/0x1b
[ 9002.455100] Code: 4d 85 ff 48 c7 c0 ea ff ff ff 0f 84 b1 00 00 00 49 8b 9d f8 01 00 00 48 85 db 0f 84 32 04 00 00 49 c7 85 f8 01 00 00 00 00 00 00 <8b> 83 70 03 00 00 4c 63 c0 89 45 cc 4b 8d 04 40 4c 8d 64 c3 70
[ 9002.455605] RIP  [<ffffffffa038c429>] __vb2_perform_fileio+0x69/0x620 [videobuf2_core]
[ 9002.455697]  RSP <ffff8801ed37fdf8>
[ 9002.455746] CR2: 0000000000000370
[ 9002.485830] ---[ end trace e865b84b28e31f5b ]---

This was found when looking for functions that return non-negative
values on error. A simplified version of the semantic match that 
finds this problem is as follows: (http://coccinelle.lip6.fr/)

// <smpl>
(
if@p1 (\(ret < 0\|ret != 0\))
 { ... return ret; }
|
ret@p1 = 0
)
... when != ret = e1
    when != &ret
*if(...)
{
  ... when != ret = e2
      when forall
 return ret;
}

// </smpl>

Signed-off-by: Peter Senna Tschudin <peter.senna@gmail.com>
---
Changes from V1:
	Updated commit message

 drivers/media/v4l2-core/videobuf2-core.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

Comments

Ezequiel Garcia Oct. 18, 2012, 3:28 p.m. UTC | #1
On Thu, Oct 18, 2012 at 11:47 AM, Peter Senna Tschudin
<peter.senna@gmail.com> wrote:
> This patch fixes a NULL pointer dereference bug at __vb2_init_fileio().
> The NULL pointer deference happens at videobuf2-core.c:
>
> static size_t __vb2_perform_fileio(struct vb2_queue *q, char __user *data, size_t count,
>                 loff_t *ppos, int nonblock, int read)
> {
> ...
>         if (!q->fileio) {
>                 ret = __vb2_init_fileio(q, read);
>                 dprintk(3, "file io: vb2_init_fileio result: %d\n", ret);
>                 if (ret)
>                         return ret;
>         }
>         fileio = q->fileio; // NULL pointer deference here
> ...
> }
>
> It was tested with vivi driver and qv4l2 for selecting read() as capture method.
> The OOPS happened when I've artificially forced the error by commenting the line:
>         if (fileio->bufs[i].vaddr == NULL)
>

... but if you manually changed the original source, how
can this be a real BUG?

Or am I missing something here ?

    Ezequiel
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Peter Senna Tschudin Oct. 18, 2012, 3:39 p.m. UTC | #2
On Thu, Oct 18, 2012 at 5:28 PM, Ezequiel Garcia <elezegarcia@gmail.com> wrote:
> On Thu, Oct 18, 2012 at 11:47 AM, Peter Senna Tschudin
> <peter.senna@gmail.com> wrote:
>> This patch fixes a NULL pointer dereference bug at __vb2_init_fileio().
>> The NULL pointer deference happens at videobuf2-core.c:
>>
>> static size_t __vb2_perform_fileio(struct vb2_queue *q, char __user *data, size_t count,
>>                 loff_t *ppos, int nonblock, int read)
>> {
>> ...
>>         if (!q->fileio) {
>>                 ret = __vb2_init_fileio(q, read);
>>                 dprintk(3, "file io: vb2_init_fileio result: %d\n", ret);
>>                 if (ret)
>>                         return ret;
>>         }
>>         fileio = q->fileio; // NULL pointer deference here
>> ...
>> }
>>
>> It was tested with vivi driver and qv4l2 for selecting read() as capture method.
>> The OOPS happened when I've artificially forced the error by commenting the line:
>>         if (fileio->bufs[i].vaddr == NULL)
>>
>
> ... but if you manually changed the original source, how
> can this be a real BUG?

It is supposed that under some circumstances, (fileio->bufs[i].vaddr
== NULL) can be true. 'While testing', my change forced the scenario
in which (fileio->bufs[i].vaddr == NULL) is true...

>
> Or am I missing something here ?
>
>     Ezequiel
On 10/18/2012 05:28 PM, Ezequiel Garcia wrote:
> On Thu, Oct 18, 2012 at 11:47 AM, Peter Senna Tschudin
> <peter.senna@gmail.com> wrote:
>> This patch fixes a NULL pointer dereference bug at __vb2_init_fileio().
>> The NULL pointer deference happens at videobuf2-core.c:
>>
>> static size_t __vb2_perform_fileio(struct vb2_queue *q, char __user *data, size_t count,
>>                 loff_t *ppos, int nonblock, int read)
>> {
>> ...
>>         if (!q->fileio) {
>>                 ret = __vb2_init_fileio(q, read);
>>                 dprintk(3, "file io: vb2_init_fileio result: %d\n", ret);
>>                 if (ret)
>>                         return ret;
>>         }
>>         fileio = q->fileio; // NULL pointer deference here
>> ...
>> }
>>
>> It was tested with vivi driver and qv4l2 for selecting read() as capture method.
>> The OOPS happened when I've artificially forced the error by commenting the line:
>>         if (fileio->bufs[i].vaddr == NULL)
>>
> 
> ... but if you manually changed the original source, how
> can this be a real BUG?
> 
> Or am I missing something here ?

He just commented out this line to trigger the bug, i.e. to simulate
a situation where fileio->bufs[i].vaddr is NULL. Which is now not
handled properly.

--
Thanks,
Sylwester
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/drivers/media/v4l2-core/videobuf2-core.c b/drivers/media/v4l2-core/videobuf2-core.c
index 432df11..dad10af 100644
--- a/drivers/media/v4l2-core/videobuf2-core.c
+++ b/drivers/media/v4l2-core/videobuf2-core.c
@@ -1878,8 +1878,10 @@  static int __vb2_init_fileio(struct vb2_queue *q, int read)
 	 */
 	for (i = 0; i < q->num_buffers; i++) {
 		fileio->bufs[i].vaddr = vb2_plane_vaddr(q->bufs[i], 0);
-		if (fileio->bufs[i].vaddr == NULL)
+		if (fileio->bufs[i].vaddr == NULL) {
+			ret = -EFAULT;
 			goto err_reqbufs;
+		}
 		fileio->bufs[i].size = vb2_plane_size(q->bufs[i], 0);
 	}