[PATCH] security: avoid calling a NULL function pointer in drivers/video/tvaudio.c

[PATCH] security: avoid calling a NULL function pointer in drivers/video/tvaudio.c

[Arjan van de Ven]

From: Arjan van de Ven <arjan@linux.intel.com>
Date: Fri, 10 Oct 2008 21:16:12 -0700
Subject: [PATCH] security: avoid calling a NULL function pointer in drivers/video/tvaudio.c

NULL function pointers are very bad security wise. This one got caught by
kerneloops.org quite a few times, so it's happening in the field....

Fix is simple, check the function pointer for NULL, like 6 other places
in the same function are already doing.

Signed-off-by: Arjan van de Ven <arjan@linux.intel.com>
---
 drivers/media/video/tvaudio.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/media/video/tvaudio.c b/drivers/media/video/tvaudio.c
index 463680b..b59e472 100644
--- a/drivers/media/video/tvaudio.c
+++ b/drivers/media/video/tvaudio.c
@@ -1792,7 +1792,7 @@ static int chip_command(struct i2c_client *client,
 		break;
 	case VIDIOC_S_FREQUENCY:
 		chip->mode = 0; /* automatic */
-		if (desc->checkmode) {
+		if (desc->checkmode && desc->setmode) {
 			desc->setmode(chip,V4L2_TUNER_MODE_MONO);
 			if (chip->prevmode != V4L2_TUNER_MODE_MONO)
 				chip->prevmode = -1; /* reset previous mode */
-- 
1.5.5.1



-- 
Arjan van de Ven 	Intel Open Source Technology Centre
For development, discussion and tips for power savings, 
visit http://www.lesswatts.org

CVE-2008-5033 - Was: Re: [PATCH] security: avoid calling a NULL function pointer in drivers/video/tvaudio.c

[Mauro Carvalho Chehab]

Hi Arjan,

On Fri, 10 Oct 2008 21:18:36 -0700
Arjan van de Ven <arjan@infradead.org> wrote:
> 
> From: Arjan van de Ven <arjan@linux.intel.com>
> Date: Fri, 10 Oct 2008 21:16:12 -0700
> Subject: [PATCH] security: avoid calling a NULL function pointer in drivers/video/tvaudio.c
> 
> NULL function pointers are very bad security wise. This one got caught by
> kerneloops.org quite a few times, so it's happening in the field....
> 
> Fix is simple, check the function pointer for NULL, like 6 other places
> in the same function are already doing.
> 
> Signed-off-by: Arjan van de Ven <arjan@linux.intel.com>
> ---
>  drivers/media/video/tvaudio.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/media/video/tvaudio.c b/drivers/media/video/tvaudio.c
> index 463680b..b59e472 100644
> --- a/drivers/media/video/tvaudio.c
> +++ b/drivers/media/video/tvaudio.c
> @@ -1792,7 +1792,7 @@ static int chip_command(struct i2c_client *client,
>  		break;
>  	case VIDIOC_S_FREQUENCY:
>  		chip->mode = 0; /* automatic */
> -		if (desc->checkmode) {
> +		if (desc->checkmode && desc->setmode) {
>  			desc->setmode(chip,V4L2_TUNER_MODE_MONO);
>  			if (chip->prevmode != V4L2_TUNER_MODE_MONO)
>  				chip->prevmode = -1; /* reset previous mode */

Unfortunately, your patch doesn't solve the bug, since desc->checkmode is
defined on all boards that defines desc->setmode callback.

Also, your patch touched on the handling for VIDIOC_S_FREQUENCY but the problem
is on another ioctl (only noticed when debug is on).

The issue is, in fact, elsewhere, as explained bellow, and seemed to be present
since the beginning.

I've successfully produced the bug with my oldest bt848 board (newer boards
generally don't use tvaudio): a STB device with tda9850 tvaudio chip.

This is the resulting OOPS:

tvaudio' 1-005b: VIDIOC_S_CTRL
BUG: unable to handle kernel NULL pointer dereference at 00000000
IP: [<00000000>]
*pde = 22fda067 *pte = 00000000
Oops: 0000 [#1] SMP
Modules linked in: snd_hda_intel snd_seq_dummy snd_seq_oss snd_seq_midi_event snd_seq snd_seq_device
snd_pcm_oss snd_mixer_oss snd_pcm snd_timer snd_hwdep snd soundcore tuner_simple tuner_types tea5767 tuner
tvaudio bttv bridgebnep rfcomm l2cap bluetooth it87 hwmon_vid hwmon fuse sunrpc ipt_REJECT
nf_conntrack_ipv4 iptable_filter ip_tables ip6t_REJECT xt_tcpudp nf_conntrack_ipv6 xt_state nf_conntrack
ip6table_filter ip6_tables x_tables ipv6 dm_mirrordm_multipath dm_mod configfs videodev v4l1_compat
ir_common 8139cp compat_ioctl32 v4l2_common 8139too videobuf_dma_sg videobuf_core mii btcx_risc tveeprom
i915 button snd_page_alloc serio_raw drm pcspkr i2c_algo_bit i2c_i801 i2c_core iTCO_wdt
iTCO_vendor_support sr_mod cdrom sg ata_generic pata_acpi ata_piix libata sd_mod scsi_mod ext3 jbdmbcache
uhci_hcd ohci_hcd ehci_hcd [last unloaded: soundcore]

Pid: 15413, comm: qv4l2 Not tainted (2.6.25.14-108.fc9.i686 #1)
EIP: 0060:[<00000000>] EFLAGS: 00210246 CPU: 0
EIP is at 0x0
EAX: 00008000 EBX: ebd21600 ECX: e2fd9ec4 EDX: 00200046
ESI: f8c0f0c4 EDI: f8c0f0c4 EBP: e2fd9d50 ESP: e2fd9d2c
 DS: 007b ES: 007b FS: 00d8 GS: 0033 SS: 0068
Process qv4l2 (pid: 15413, ti=e2fd9000 task=ebe44000 task.ti=e2fd9000)
Stack: f8c0c6ae e2ff2a00 00000d00 e2fd9ec4 ebc4e000 e2fd9d5c f8c0c448 00000000
       f899c12a e2fd9d5c f899c154 e2fd9d68 e2fd9d80 c0560185 e2fd9d88 f8f3e1d8
       f8f3e1dc ebc4e034 f8f3e18c e2fd9ec4 00000000 e2fd9d90 f899c286 c008561c
Call Trace:
 [<f8c0c6ae>] ? chip_command+0x266/0x4b6 [tvaudio]
 [<f8c0c448>] ? chip_command+0x0/0x4b6 [tvaudio]
 [<f899c12a>] ? i2c_cmd+0x0/0x2f [i2c_core]
 [<f899c154>] ? i2c_cmd+0x2a/0x2f [i2c_core]
 [<c0560185>] ? device_for_each_child+0x21/0x49
 [<f899c286>] ? i2c_clients_command+0x1c/0x1e [i2c_core]
 [<f8f283d8>] ? bttv_call_i2c_clients+0x14/0x16 [bttv]
 [<f8f23601>] ? bttv_s_ctrl+0x1bc/0x313 [bttv]
 [<f8f23445>] ? bttv_s_ctrl+0x0/0x313 [bttv]
 [<f8b6096d>] ? __video_do_ioctl+0x1f84/0x3726 [videodev]
 [<c05abb4e>] ? sock_aio_write+0x100/0x10d
 [<c041b23e>] ? kmap_atomic_prot+0x1dd/0x1df
 [<c043a0c9>] ? enqueue_hrtimer+0xc2/0xcd
 [<c04f4fa4>] ? copy_from_user+0x39/0x121
 [<f8b622b9>] ? __video_ioctl2+0x1aa/0x24a [videodev]
 [<c04054fd>] ? do_notify_resume+0x768/0x795
 [<c043c0f7>] ? getnstimeofday+0x34/0xd1
 [<c0437b77>] ? autoremove_wake_function+0x0/0x33
 [<f8b62368>] ? video_ioctl2+0xf/0x13 [videodev]
 [<c048c6f0>] ? vfs_ioctl+0x50/0x69
 [<c048c942>] ? do_vfs_ioctl+0x239/0x24c
 [<c048c995>] ? sys_ioctl+0x40/0x5b
 [<c0405bf2>] ? syscall_call+0x7/0xb
 [<c0620000>] ? cpuid4_cache_sysfs_exit+0x3d/0x69
 =======================
Code:  Bad EIP value.
EIP: [<00000000>] 0x0 SS:ESP 0068:e2fd9d2c

You should notice that, just before the bug, that the driver received a
VIDIOC_S_CTRL internal ioctl.

After digging at this error, I noticed the cause: The logic that checks for the
existence of bassfunc() and treblefunc() callback.

Instead of doing a direct check, the driver checks it by looking at
desc->flags. if desc->flags & HAS_BASSTREBLE, then tvaudio tries to call the
callbacks for bass and treble.

However, the logic were inverted on some places.

The following patch properly fixes this trouble. I've made it available at my
-git tree[1], together with other tvaudio patches meant to reduce potential issues
at tvaudio.

In order to avoid some risks like defining the flag without providing the
callbacks, I've added other patches at the series to check if the callbacks
exist at the init code.

I'm sending a separate email with the pull requests for the entire series of
tvaudio patches.

[1] http://git.kernel.org/?p=linux/kernel/git/mchehab/linux-2.6.git;a=commitdiff;h=01a1a3cc1e3fbe718bd06a2a5d4d1a2d0fb4d7d9

Cheers,
Mauro

---

This is the patch that actually solved the above bug. The problematic code that
generated the OOPS is at the second hunk, when the driver calls
desc->bassfunc(chip->bass).

diff --git a/drivers/media/video/tvaudio.c b/drivers/media/video/tvaudio.c
index fb46ce4..3720f0e 100644
--- a/drivers/media/video/tvaudio.c
+++ b/drivers/media/video/tvaudio.c
@@ -1639,13 +1639,13 @@ static int tvaudio_get_ctrl(struct CHIPSTATE *chip,
 		return 0;
 	}
 	case V4L2_CID_AUDIO_BASS:
-		if (desc->flags & CHIP_HAS_BASSTREBLE)
+		if (!(desc->flags & CHIP_HAS_BASSTREBLE))
 			break;
 		ctrl->value = chip->bass;
 		return 0;
 	case V4L2_CID_AUDIO_TREBLE:
-		if (desc->flags & CHIP_HAS_BASSTREBLE)
-			return -EINVAL;
+		if (!(desc->flags & CHIP_HAS_BASSTREBLE))
+			break;
 		ctrl->value = chip->treble;
 		return 0;
 	}
@@ -1705,16 +1705,15 @@ static int tvaudio_set_ctrl(struct CHIPSTATE *chip,
 		return 0;
 	}
 	case V4L2_CID_AUDIO_BASS:
-		if (desc->flags & CHIP_HAS_BASSTREBLE)
+		if (!(desc->flags & CHIP_HAS_BASSTREBLE))
 			break;
 		chip->bass = ctrl->value;
 		chip_write(chip,desc->bassreg,desc->bassfunc(chip->bass));
 
 		return 0;
 	case V4L2_CID_AUDIO_TREBLE:
-		if (desc->flags & CHIP_HAS_BASSTREBLE)
-			return -EINVAL;
-
+		if (!(desc->flags & CHIP_HAS_BASSTREBLE))
+			break;
 		chip->treble = ctrl->value;
 		chip_write(chip,desc->treblereg,desc->treblefunc(chip->treble));
 
@@ -1761,7 +1760,7 @@ static int chip_command(struct i2c_client *client,
 				break;
 			case V4L2_CID_AUDIO_BASS:
 			case V4L2_CID_AUDIO_TREBLE:
-				if (desc->flags & CHIP_HAS_BASSTREBLE)
+				if (!(desc->flags & CHIP_HAS_BASSTREBLE))
 					return -EINVAL;
 				break;
 			default:

Cheers,
Mauro