2003-05-01 04:27:05

by Junfeng Yang

[permalink] [raw]
Subject: [CHECKER] 5 potential user-pointer errors that allow arbitrary reads from kernel

Hi,

This is a resend (the previous report was ignored, however I feel that
these bugs could be severe).

Here are 5 bugs in 2.5.63 where user pointers are passed into memcpy or
simple_strtoul without verifications. These bugs appear functions assigned
to struct proc_dir_entry.write_proc, where a malicious user can call
write_proc on a arbitrary pointer pointing to any sensitive kernel data,
then call the corresponding read_proc to get back the data.

Please confirm or clarify. Thanks!

-Junfeng

---------------------------------------------------------
[BUG] proc_dir_entry.write_proc can take tainted inputs

/home/junfeng/linux-2.5.63/drivers/usb/media/vicam.c:1117:vicam_write_proc_gain:
ERROR:TAINTED:1117:1117: passing tainted ptr 'buffer' to simple_strtoul
[Callstack:
/home/junfeng/linux-2.5.63/net/core/pktgen.c:991:vicam_write_proc_gain((tainted
1))]

static int vicam_write_proc_gain(struct file *file, const char *buffer,
unsigned long count, void *data)
{
struct vicam_camera *cam = (struct vicam_camera *)data;


Error --->
cam->gain = simple_strtoul(buffer, NULL, 10);

return count;
}
---------------------------------------------------------
[BUG] proc_dir_entry.write_proc can take tainted inputs

/home/junfeng/linux-2.5.63/drivers/usb/media/vicam.c:1107:vicam_write_proc_shutter:
ERROR:TAINTED:1107:1107: passing tainted ptr 'buffer' to simple_strtoul
[Callstack:
/home/junfeng/linux-2.5.63/net/core/pktgen.c:991:vicam_write_proc_shutter((tainted
1))]

static int vicam_write_proc_shutter(struct file *file, const char *buffer,
unsigned long count, void *data)
{
struct vicam_camera *cam = (struct vicam_camera *)data;


Error --->
cam->shutter_speed = simple_strtoul(buffer, NULL, 10);

return count;
}
---------------------------------------------------------
[BUG] proc_dir_entry.write_proc

/home/junfeng/linux-2.5.63/drivers/media/video/zoran_procfs.c:122:zoran_write_proc:
ERROR:TAINTED:122:122: passing tainted ptr 'buffer' to __memcpy
[Callstack:
/home/junfeng/linux-2.5.63/net/core/pktgen.c:991:zoran_write_proc((tainted
1))]

string = sp = vmalloc(count + 1);
if (!string) {
printk(KERN_ERR "%s: write_proc: can not allocate
memory\n", zr->name);
return -ENOMEM;
}

Error --->
memcpy(string, buffer, count);
string[count] = 0;
DEBUG2(printk(KERN_INFO "%s: write_proc: name=%s count=%lu
data=%x\n", zr->name, file->f_dentry->d_name.name, count, (int) data));
ldelim = " \t\n";
---------------------------------------------------------
[BUG] proc_dir_entry.write_proc

/home/junfeng/linux-2.5.63/drivers/pnp/pnpbios/proc.c:190:proc_write_node:
ERROR:TAINTED:190:190: passing tainted ptr 'buf' to __memcpy [Callstack:
/home/junfeng/linux-2.5.63/net/core/pktgen.c:991:proc_write_node((tainted
1))]

if (!node) return -ENOMEM;
if ( pnp_bios_get_dev_node(&nodenum, boot, node) )
return -EIO;
if (count != node->size - sizeof(struct pnp_bios_node))
return -EINVAL;

Error --->
memcpy(node->data, buf, count);
if (pnp_bios_set_dev_node(node->handle, boot, node) != 0)
return -EINVAL;
kfree(node);
---------------------------------------------------------
[BUG] proc_dir_entry.write_proc can take tainted inputs.
av7110_ir_write_proc is assigned to proc_dir_entry.write_proc

/home/junfeng/linux-2.5.63/drivers/media/dvb/av7110/av7110_ir.c:116:av7110_ir_write_proc:
ERROR:TAINTED:116:116: passing tainted ptr 'buffer' to __constant_memcpy
[Callstack:
/home/junfeng/linux-2.5.63/net/core/pktgen.c:991:av7110_ir_write_proc((tainted
1))]

u32 ir_config;

if (count < 4 + 256 * sizeof(u16))
return -EINVAL;


Error --->
memcpy (&ir_config, buffer, 4);
memcpy (&key_map, buffer + 4, 256 * sizeof(u16));

av7110_setup_irc_config (NULL, ir_config);




2003-05-01 04:42:59

by Junfeng Yang

[permalink] [raw]
Subject: [CHECKER] 2 potential passing kernel-pointer into copy_*_user errors


Hi,

Below are 2 more warnings where kernel pointer is passed into *_do_ioctl
(these functions are passed into video_usercopy). Please note that our
checker flags the dereferences as errors, where the actually errors should
be the copy_*_user calls.

Thanks a lot!

-Junfeng

---------------------------------------------------------
[BUG] pass kernel pointer into copy_*_user. bug is in VIDIOCGTUNER. Should
not call copy_to_user on arg since arg is already in kernel space.

/home/junfeng/linux-2.5.63/drivers/media/radio/radio-cadet.c:397:cadet_do_ioctl:
ERROR:TAINTED:397:397: dereferencing tainted ptr 'v' [Callstack: ]

{
case VIDIOCGCAP:
{
struct video_capability *v = arg;
memset(v,0,sizeof(*v));

Error --->
v->type=VID_TYPE_TUNER;
v->channels=2;
v->audios=1;
strcpy(v->name, "ADS Cadet");
---------------------------------------------------------
[BUG] pass kernel pointer into copy_*_user. should not call copy_to_user
on case VIDIOCGCHAN

/home/junfeng/linux-2.5.63/drivers/media/video/bw-qcam.c:763:qcam_do_ioctl:
ERROR:TAINTED:763:763: dereferencing tainted ptr 'p' [Callstack: ]

return 0;
}
case VIDIOCGPICT:
{
struct video_picture *p = arg;

Error --->
p->colour=0x8000;
p->hue=0x8000;
p->brightness=qcam->brightness<<8;
p->contrast=qcam->contrast<<8;


2003-05-01 05:33:40

by Junfeng Yang

[permalink] [raw]
Subject: Re: [CHECKER] 5 potential user-pointer errors that allow arbitrary reads from kernel


Also, please note the word "Callstack" in the error reports doesn't really
mean an actual call here. It means "functions are linked together if they
are assigned to the same structure field".

for example, the following error report is catched in this way:

net/core/pktgen.c:proc_write treats its second arguemtn as tainted.
we take it as a good code example, and require that all the other
functions that are assigned to proc_dir_entry.write_proc must treat its
second argument as tainted.

sorry for the confusions.

> ---------------------------------------------------------
> [BUG] proc_dir_entry.write_proc can take tainted inputs
>
> /home/junfeng/linux-2.5.63/drivers/usb/media/vicam.c:1117:vicam_write_proc_gain:
> ERROR:TAINTED:1117:1117: passing tainted ptr 'buffer' to simple_strtoul
> [Callstack:
> /home/junfeng/linux-2.5.63/net/core/pktgen.c:991:vicam_write_proc_gain((tainted
> 1))]
>
> static int vicam_write_proc_gain(struct file *file, const char *buffer,
> unsigned long count, void *data)
> {
> struct vicam_camera *cam = (struct vicam_camera *)data;
>
>
> Error --->
> cam->gain = simple_strtoul(buffer, NULL, 10);
>
> return count;
> }

2003-05-01 12:42:05

by Michael Hunold

[permalink] [raw]
Subject: Re: [CHECKER] 5 potential user-pointer errors that allow arbitrary reads from kernel

Hello Junfeng,

> This is a resend (the previous report was ignored, however I feel that
> these bugs could be severe).

> Please confirm or clarify. Thanks!

> [BUG] proc_dir_entry.write_proc can take tainted inputs.
> av7110_ir_write_proc is assigned to proc_dir_entry.write_proc
>
> /home/junfeng/linux-2.5.63/drivers/media/dvb/av7110/av7110_ir.c:116:av7110_ir_write_proc:
> ERROR:TAINTED:116:116: passing tainted ptr 'buffer' to __constant_memcpy
> [Callstack:
> /home/junfeng/linux-2.5.63/net/core/pktgen.c:991:av7110_ir_write_proc((tainted
> 1))]

Confirmed. I'll post a patch when I'm back at work again on Monday.

CU
Michael.

2003-05-01 19:55:25

by Junfeng Yang

[permalink] [raw]
Subject: Re: [CHECKER] 5 potential user-pointer errors that allow arbitrary reads from kernel


Thanks a lot for the feedback and the incoming patch!

-Junfeng

On Thu, 1 May 2003, Michael Hunold wrote:

> Hello Junfeng,
>
> > This is a resend (the previous report was ignored, however I feel that
> > these bugs could be severe).
>
> > Please confirm or clarify. Thanks!
>
> > [BUG] proc_dir_entry.write_proc can take tainted inputs.
> > av7110_ir_write_proc is assigned to proc_dir_entry.write_proc
> >
> > /home/junfeng/linux-2.5.63/drivers/media/dvb/av7110/av7110_ir.c:116:av7110_ir_write_proc:
> > ERROR:TAINTED:116:116: passing tainted ptr 'buffer' to __constant_memcpy
> > [Callstack:
> > /home/junfeng/linux-2.5.63/net/core/pktgen.c:991:av7110_ir_write_proc((tainted
> > 1))]
>
> Confirmed. I'll post a patch when I'm back at work again on Monday.
>
> CU
> Michael.
>

2003-05-01 20:38:12

by Greg KH

[permalink] [raw]
Subject: Re: [CHECKER] 5 potential user-pointer errors that allow arbitrary reads from kernel

On Wed, Apr 30, 2003 at 09:39:18PM -0700, Junfeng Yang wrote:
> ---------------------------------------------------------
> [BUG] proc_dir_entry.write_proc can take tainted inputs
>
> /home/junfeng/linux-2.5.63/drivers/usb/media/vicam.c:1117:vicam_write_proc_gain:
> ERROR:TAINTED:1117:1117: passing tainted ptr 'buffer' to simple_strtoul
> [Callstack:
> /home/junfeng/linux-2.5.63/net/core/pktgen.c:991:vicam_write_proc_gain((tainted
> 1))]
>
> static int vicam_write_proc_gain(struct file *file, const char *buffer,
> unsigned long count, void *data)
> {
> struct vicam_camera *cam = (struct vicam_camera *)data;
>
>
> Error --->
> cam->gain = simple_strtoul(buffer, NULL, 10);

Real bug, I'll fix this.

> ---------------------------------------------------------
> [BUG] proc_dir_entry.write_proc can take tainted inputs
>
> /home/junfeng/linux-2.5.63/drivers/usb/media/vicam.c:1107:vicam_write_proc_shutter:
> ERROR:TAINTED:1107:1107: passing tainted ptr 'buffer' to simple_strtoul
> [Callstack:
> /home/junfeng/linux-2.5.63/net/core/pktgen.c:991:vicam_write_proc_shutter((tainted
> 1))]
>
> static int vicam_write_proc_shutter(struct file *file, const char *buffer,
> unsigned long count, void *data)
> {
> struct vicam_camera *cam = (struct vicam_camera *)data;
>
>
> Error --->
> cam->shutter_speed = simple_strtoul(buffer, NULL, 10);

Again, real bug, I'll fix it.

thanks,

greg k-h

2003-05-01 20:41:45

by Junfeng Yang

[permalink] [raw]
Subject: Re: [CHECKER] 5 potential user-pointer errors that allow arbitrary reads from kernel


Thanks!

On Thu, 1 May 2003, Greg KH wrote:

> On Wed, Apr 30, 2003 at 09:39:18PM -0700, Junfeng Yang wrote:
> > ---------------------------------------------------------
> > [BUG] proc_dir_entry.write_proc can take tainted inputs
> >
> > /home/junfeng/linux-2.5.63/drivers/usb/media/vicam.c:1117:vicam_write_proc_gain:
> > ERROR:TAINTED:1117:1117: passing tainted ptr 'buffer' to simple_strtoul
> > [Callstack:
> > /home/junfeng/linux-2.5.63/net/core/pktgen.c:991:vicam_write_proc_gain((tainted
> > 1))]
> >
> > static int vicam_write_proc_gain(struct file *file, const char *buffer,
> > unsigned long count, void *data)
> > {
> > struct vicam_camera *cam = (struct vicam_camera *)data;
> >
> >
> > Error --->
> > cam->gain = simple_strtoul(buffer, NULL, 10);
>
> Real bug, I'll fix this.
>
> > ---------------------------------------------------------
> > [BUG] proc_dir_entry.write_proc can take tainted inputs
> >
> > /home/junfeng/linux-2.5.63/drivers/usb/media/vicam.c:1107:vicam_write_proc_shutter:
> > ERROR:TAINTED:1107:1107: passing tainted ptr 'buffer' to simple_strtoul
> > [Callstack:
> > /home/junfeng/linux-2.5.63/net/core/pktgen.c:991:vicam_write_proc_shutter((tainted
> > 1))]
> >
> > static int vicam_write_proc_shutter(struct file *file, const char *buffer,
> > unsigned long count, void *data)
> > {
> > struct vicam_camera *cam = (struct vicam_camera *)data;
> >
> >
> > Error --->
> > cam->shutter_speed = simple_strtoul(buffer, NULL, 10);
>
> Again, real bug, I'll fix it.
>
> thanks,
>
> greg k-h
>

2003-05-02 06:31:06

by Junfeng Yang

[permalink] [raw]
Subject: [CHECKER] 4 potential user-pointer errors


Hi,

Enclosed are 4 more potential bugs where user pointer is dereferenced.
Please note the word "Callstack" in the bug report doesn't always mean a
call. We link functions together if they are assigned to the same struct
field.

Please confirm or clarify. Thanks!

-Junfeng

---------------------------------------------------------
[BUG] at least bad programming practice. file_operations.write ->
cm_write -> trans_ac3. write can take tainted. write can take tainted
inputs. the pointer is vefied in cm_write

/home/junfeng/linux-2.5.63/sound/oss/cmpci.c:593:trans_ac3:
ERROR:TAINTED:593:593: dereferencing tainted ptr 'src' [Callstack:
/home/junfeng/linux-2.5.63/fs/read_write.c:307:vfs_write((tainted
1)(tainted 2)) ->
/home/junfeng/linux-2.5.63/fs/read_write.c:241:cm_write((tainted
1)(tainted 2)) ->
/home/junfeng/linux-2.5.63/sound/oss/cmpci.c:1662:trans_ac3((tainted
2))]

unsigned long data;
unsigned long *dst = (unsigned long *) dest;
unsigned short *src = (unsigned short *)source;

do {

Error --->
data = (unsigned long) *src++;
data <<= 12; // ok for 16-bit data
if (s->spdif_counter == 2 || s->spdif_counter == 3)
data |= 0x40000000; // indicate AC-3 raw
data
---------------------------------------------------------
[BUG] at least bad programming practice. file_opetations.ioctl ->
aac_cfg_ioctl -> aac_do_ioctl -> close_getadapter_fib ->
aac_close_fib_context. All other functions called by aac_do_ioctl assume
arg is a user pointer. It is unknown what will happen.

/home/junfeng/linux-2.5.63/drivers/scsi/aacraid/commctrl.c:277:aac_close_fib_context:
ERROR:TAINTED:277:277: dereferencing tainted ptr 'fibctx' [Callstack:
/home/junfeng/linux-2.5.63/drivers/scsi/sg.c:1002:aac_cfg_ioctl((tainted
3)) ->
/home/junfeng/linux-2.5.63/drivers/scsi/aacraid/linit.c:671:aac_do_ioctl((tainted
2)) ->
/home/junfeng/linux-2.5.63/drivers/scsi/aacraid/commctrl.c:421:close_getadapter_fib((tainted
1)) ->
/home/junfeng/linux-2.5.63/drivers/scsi/aacraid/commctrl.c:348:aac_close_fib_context((tainted
1))]

while (!list_empty(&fibctx->fibs)) {
struct list_head * entry;
/*
* Pull the next fib from the fibs
*/

Error --->
entry = fibctx->fibs.next;
list_del(entry);
fib = list_entry(entry, struct hw_fib, header.FibLinks);
fibctx->count--;
---------------------------------------------------------
[BUG] at least bad programming practice. zoran_read and vbi_read are
both assigned to video_device.read, while zoran_read assumes "buf" is
tainted. The pointer is verified by access_ok

/home/junfeng/linux-2.5.63/drivers/media/video/zr36120.c:1698:vbi_read:
ERROR:TAINTED:1698:1698: dereferencing tainted ptr 'optr' [Callstack:
/home/junfeng/linux-2.5.63/drivers/media/video/zr36120.c:934:vbi_read((tainted
1))]

{
/* copy to doubled data to userland */
for (x=0; optr+1<eptr && x<-done->w; x++)
{
unsigned char a = iptr[x*2];

Error --->
*optr++ = a;
*optr++ = a;
}
/* and clear the rest of the line */
---------------------------------------------------------
[BUG] at least bad programming practice. calls access_ok on
qv.packet_sizes, then passed qv.packet_sizes into
initialize_dma_it_prg_var_packet_queue.

/home/junfeng/linux-2.5.63/drivers/ieee1394/video1394.c:668:initialize_dma_it_prg_var_packet_queue:
ERROR:TAINTED:668:668: dereferencing tainted ptr 'packet_sizes + i * 4'
[Callstack:
/home/junfeng/linux-2.5.63/drivers/ieee1394/video1394.c:1047:initialize_dma_it_prg_var_packet_queue((tainted
2))]

for (i = 0; i < d->nb_cmd; i++) {
unsigned int size;
if (packet_sizes[i] > d->packet_size) {
size = d->packet_size;
} else {

Error --->
size = packet_sizes[i];
}
it_prg[i].data[1] = cpu_to_le32(size << 16);
it_prg[i].end.control = cpu_to_le32(DMA_CTL_OUTPUT_LAST
| DMA_CTL_BRANCH);

2003-05-09 21:32:08

by Junfeng Yang

[permalink] [raw]
Subject: [CHECKER] Clarifications needed on a user-pointer false alarm in kernel/kmod.c


Hi,

I got the following false alarm in kernel/kmod.c.

the call chain is sys_wait4 (_, &sub_info->retval) -> wait_task_zombie (_,
_, stat_addr, _) -> put_user (_, stat_addr), which means &sub_info->retval
will be passed into put_user. From the calling context, sub_info should be
in kernel space, so &sub_info->retval should be in kernel space as well.
The explanation for this false alarm could be that the call chain wasn't
realistic, but I'm not sure. Can you guys please help me on that?

/home/junfeng/linux-tainted/kernel/kmod.c:185:wait_for_helper:
ERROR:TAINTED:185:185: dereferencing tainted ptr 'sub_info' [Callstack: ]
if (pid < 0)
sub_info->retval = pid;
else
sys_wait4(pid, (unsigned int *)&sub_info->retval, 0, NULL);


Error --->
complete(sub_info->complete);
return 0;
}


2003-05-12 06:17:13

by Junfeng Yang

[permalink] [raw]
Subject: [CHECKER] 1 potential derefence of user-pointer without verification error


Hi,

Below is a warning found in pcmcia/ds.c, where user pointer is
dereferenced without check. Please confirm or clarify, Thanks!

-Junfeng

---------------------------------------------------------

[BUG] buf is tainted implies buf.win_info.handle is tainted.
pcmcia_get_mem_page dereferences its first parameter

/home/junfeng/linux-tainted/drivers/pcmcia/ds.c:814:ds_ioctl:
ERROR:TAINTED: 814:814:deref tainted component 'buf.win_info.handle'
[struct=win_info_t.handle] [type=call]

break;
case DS_GET_NEXT_WINDOW:
ret = pcmcia_get_next_window(&buf.win_info.handle,
&buf.win_info.window);
break;
case DS_GET_MEM_PAGE:

Error --->
ret = pcmcia_get_mem_page(buf.win_info.handle,
&buf.win_info.map);
break;
case DS_REPLACE_CIS:


2003-05-12 06:31:38

by Junfeng Yang

[permalink] [raw]
Subject: Re: [CHECKER] 1 potential derefence of user-pointer without verification error


here is a detailed explanation in case the warnning itself isn't clear:

1. ds_ioctl is assigned to file_operantions.ioctl
so its argument 'arg' is tainted. verify_area are
also called on 'arg', which confirms.

2. copy_from_user (&buf, arg, _) copies in the content of arg

3. buf.win_info.handle is thus a user provided pointer.

4. pcmcia_get_mem_page dereferences its first parameter, in this case
buf.win_info.handle

-Junfeng

On Sun, 11 May 2003, Junfeng Yang wrote:

>
> Hi,
>
> Below is a warning found in pcmcia/ds.c, where user pointer is
> dereferenced without check. Please confirm or clarify, Thanks!
>
> -Junfeng
>
> ---------------------------------------------------------
>
> [BUG] buf is tainted implies buf.win_info.handle is tainted.
> pcmcia_get_mem_page dereferences its first parameter
>
> /home/junfeng/linux-tainted/drivers/pcmcia/ds.c:814:ds_ioctl:
> ERROR:TAINTED: 814:814:deref tainted component 'buf.win_info.handle'
> [struct=win_info_t.handle] [type=call]
>
> break;
> case DS_GET_NEXT_WINDOW:
> ret = pcmcia_get_next_window(&buf.win_info.handle,
> &buf.win_info.window);
> break;
> case DS_GET_MEM_PAGE:
>
> Error --->
> ret = pcmcia_get_mem_page(buf.win_info.handle,
> &buf.win_info.map);
> break;
> case DS_REPLACE_CIS:
>
>

2003-05-12 21:07:20

by Junfeng Yang

[permalink] [raw]
Subject: [CHECKER] One more potential user-pointer error


Hi,

Enclosed is one warning in drivers/char/drm/radeon_state.c I've attached
a detaillsed explanation. If it is not clear, please mail me.

As always, please confirm or clarify. Thanks!

-Junfeng

---------------------------------------------------------

[BUG] function radeon_cp_dispatch_indices calls
DRM_COPY_FROM_USER_UNCHECKED on parameter 'dev_priv->sarea_priv->boxes',
implies it is a user space pointer. dev_piv->sarea_priv has type
'drm_radeon_sarea_t', where field 'drm_radeon_sarea_t.boxes' is declared
as an array. so dev_priv->sarea_priv->boxes is equivalent to
dev_priv->sarea_priv + offset of field 'boxes'. since dev_priv->sarea_priv
+ offset_of_'boxes' is tainted, dev_priv->sarea_priv is also a user-space
pointer. this pointer is deref'd several times.

/home/junfeng/linux-tainted/drivers/char/drm/radeon_state.c:1554:radeon_cp_indices:
ERROR:TAINTED:1554:1554: dereferencing tainted ptr 'dev_priv->sarea_priv'
[Callstack: ]

prim.prim = elts.prim;
prim.offset = 0; /* offset from start of dma buffers */
prim.numverts = RADEON_MAX_VB_VERTS; /* duh */
prim.vc_format = dev_priv->sarea_priv->vc_format;


Error --->
radeon_cp_dispatch_indices( dev, buf, &prim,
dev_priv->sarea_priv->boxes,
dev_priv->sarea_priv->nbox );
if (elts.discard) {
---------------------------------------------------------
[BUG]

/home/junfeng/linux-tainted/drivers/char/drm/radeon_state.c:1773:radeon_cp_vertex2:
ERROR:TAINTED:1773:1773: dereferencing tainted ptr 'sarea_priv'
[Callstack: ]


if ( prim.prim & RADEON_PRIM_WALK_IND ) {
tclprim.offset = prim.numverts * 64;
tclprim.numverts = RADEON_MAX_VB_VERTS; /* duh */


Error --->
radeon_cp_dispatch_indices( dev, buf, &tclprim,
sarea_priv->boxes,
sarea_priv->nbox);
} else {
---------------------------------------------------------
[BUG]

/home/junfeng/linux-tainted/drivers/char/drm/radeon_state.c:1454:radeon_cp_vertex:
ERROR:TAINTED:1454:1454: dereferencing tainted ptr 'dev_priv->sarea_priv'
[Callstack: ]

prim.finish = vertex.count; /* unused */
prim.prim = vertex.prim;
prim.numverts = vertex.count;
prim.vc_format = dev_priv->sarea_priv->vc_format;


Error --->
radeon_cp_dispatch_vertex( dev, buf, &prim,
dev_priv->sarea_priv->boxes,
dev_priv->sarea_priv->nbox );
}


2003-05-15 20:28:00

by Junfeng Yang

[permalink] [raw]
Subject: [CHECKER] 2 potential out-of-bound user-pointer errors in fs/readdir.c


Hi,

Enclosed are two warnings found in fs/readdir.c, where user provided
pointers are accessed out of 'verified' bounds.

The warnings are found by: first, whenenver we see calls to verify_area,
access_ok and all the no-underscore versions of *_user functions, we
remember the verified bounds. when a user-pointer is accessed thru
__*_user functions, we check if the verified bound is big enough.

Please confirm or clarify. Thanks!

-Junfeng


---------------------------------------------------------

[BUG] copy_to_user verifies that [0, namelen) of dirent->d_name is okay to
write, but the following __put_user accesses dirent->d_name + namelen

/home/junfeng/linux-2.5.69/fs/readdir.c:239:filldir64:
ERROR:BUFFER:239:239:memory operation error (len < off + n) (__put_user(0,
(void*)(char*)&dirent->d_name + (char*)(long unsigned
int)namlen):(void*)(char*)&dirent->d_name + (char*)(long unsigned
int)namlen, len = sym_8, off = sym_8 + 0, n = 1, min (off + n - len = 1)

goto efault;
if (__put_user(d_type, &dirent->d_type))
goto efault;
if (copy_to_user(dirent->d_name, name, namlen))
goto efault;

Error --->
if (__put_user(0, dirent->d_name + namlen))
goto efault;
((char *) dirent) += reclen;
buf->current_dir = dirent;
---------------------------------------------------------

[BUG] copy_to_user verifies that [0, namelen) of dirent->d_name is okay to
write, but the following __put_user accesses dirent->d_name + namelen

/home/junfeng/linux-2.5.69/fs/readdir.c:154:filldir:
ERROR:BUFFER:154:154:memory operation error (len < off + n) (__put_user(0,
(void*)(char*)&dirent->d_name + (char*)(long unsigned
int)namlen):(void*)(char*)&dirent->d_name + (char*)(long unsigned
int)namlen, len = sym_8, off = sym_8 + 0, n = 1, min (off + n - len = 1)

goto efault;
if (__put_user(reclen, &dirent->d_reclen))
goto efault;
if (copy_to_user(dirent->d_name, name, namlen))
goto efault;

Error --->
if (__put_user(0, dirent->d_name + namlen))
goto efault;
((char *) dirent) += reclen;
buf->current_dir = dirent;


2003-05-15 21:55:38

by Andrew Morton

[permalink] [raw]
Subject: Re: [CHECKER] 2 potential out-of-bound user-pointer errors in fs/readdir.c

Junfeng Yang <[email protected]> wrote:
>
>
> Hi,
>
> Enclosed are two warnings found in fs/readdir.c, where user provided
> pointers are accessed out of 'verified' bounds.
>
> The warnings are found by: first, whenenver we see calls to verify_area,
> access_ok and all the no-underscore versions of *_user functions, we
> remember the verified bounds. when a user-pointer is accessed thru
> __*_user functions, we check if the verified bound is big enough.
>
> Please confirm or clarify. Thanks!

The code as-is appears to be OK. Note how sys_getdents64() will run
access_ok() against the entire user buffer up-front. Then the start/len of
that verified area is copied into the getdents_callback64 and that is
propagated down to filldir64().

And filldir64() looks like it correctly remains within the bounds of the
start/len.

I guess that copy_to_user() should be __copy_to_user().


2003-05-16 00:16:26

by Junfeng Yang

[permalink] [raw]
Subject: Re: [CHECKER] 2 potential out-of-bound user-pointer errors in fs/readdir.c


Thanks for the clarifications! so these are not real security bugs, but
redundant checks.

-Junfeng

On Thu, 15 May 2003, Andrew Morton wrote:

> Junfeng Yang <[email protected]> wrote:
> >
> >
> > Hi,
> >
> > Enclosed are two warnings found in fs/readdir.c, where user provided
> > pointers are accessed out of 'verified' bounds.
> >
> > The warnings are found by: first, whenenver we see calls to verify_area,
> > access_ok and all the no-underscore versions of *_user functions, we
> > remember the verified bounds. when a user-pointer is accessed thru
> > __*_user functions, we check if the verified bound is big enough.
> >
> > Please confirm or clarify. Thanks!
>
> The code as-is appears to be OK. Note how sys_getdents64() will run
> access_ok() against the entire user buffer up-front. Then the start/len of
> that verified area is copied into the getdents_callback64 and that is
> propagated down to filldir64().
>
> And filldir64() looks like it correctly remains within the bounds of the
> start/len.
>
> I guess that copy_to_user() should be __copy_to_user().
>
>