[lkml]   [2009]   [Oct]   [10]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
Messages in this thread
SubjectRe: [patch 37/37] sit: fix off-by-one in ipip6_tunnel_get_prl
On Saturday 10 October 2009, David Miller wrote:
> From: "Templin, Fred L" <>
> Date: Fri, 9 Oct 2009 17:34:49 -0700
> > Wait a moment - I remember now that this code came
> > from Yoshifuji, and I believe there was a reason for
> > the cmax+1. The application is expected to know this
> > and to post a large enough buffer.

But wouldn't this corrupt kernel memory?

It seems:

kp is never larger then

sizeof(struct ip_tunnel_prl) * cmax

so kp[cmax] is of by one.

userspace can't allocate a large enough buffer as doesn't know the size of the

If the comment is correct and non-root can call this function it could set
a.addr = htonl(INADDR_ANY);
a.datalen = sizeof(*kp);

=> cmax == 1
=> kp[1] would be written if prl has 2 or more entries.

So I think the patch is correct.

Probably it would even be better to check for

c >= ca

as this even more obviously correct.

> >
> > Can we put this on hold until I have had a chance to
> > check my e-mail archives and my local iproute changes
> > (will respond on monday)?
> Sure, we can keep it out of -stable for now.
> But it is in Linus's tree so if you find we shouldn't do this
> you'll need to send me a revert for net-2.6
> Otherwise if it's good, you'll have to remind me to resubmit
> it to -stable.

This function seems to be a bit strange:


if (!kp) {
/* We don't try hard to allocate much memory for
* non-root users.
* For root users, retry allocating enough memory for
* the answer.
kp = kcalloc(ca, sizeof(*kp), GFP_ATOMIC);
if (!kp) {
ret = -ENOMEM;
goto out;

why ist ret set to -ENOMEM
it will be set to 0 here:


len = sizeof(*kp) * c;
ret = 0;

Because c == 0 len will be 0, so only
put_user(len, &a->datalen)

will be executed
I'm not sure what this means for put_user(len, &a->datalen) but probably it
will succed. Therefor the return value is 0 in this case.

I think the

ret = 0

should be removed as ret is initialized to 0 so it will be zero if not

And either

if (! ret) {
len = sizeof(*kp) * c;
if ((len && copy_to_user(a + 1, kp, len)) || put_user(len, &a->datalen))
ret = -EFAULT;


len = sizeof(*kp) * c;
if (len && (opy_to_user(a + 1, kp, len) || put_user(len, &a->datalen))
ret = -EFAULT;

2) Then

c = 0;
for (prl = t->prl; prl; prl = prl->next) {

c is already 0 here, so this seems to be unecessary

3) If the caller should know that the list is to large for the buffer he
supplied by returning -EFAULT then cmax should be increased by one:

cmax = kprl.datalen / sizeof(kprl) + 1;

But probably user space should initialise its buffer with 0 and assume that the
list was to large if the last entry's addr != 0.

Wolfgang Walter
Studentenwerk München
Anstalt des öffentlichen Rechts
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to
More majordomo info at
Please read the FAQ at

 \ /
  Last update: 2009-10-11 03:47    [W:0.108 / U:2.176 seconds]
©2003-2018 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site