linux-kernel - RE: [patch 37/37] sit: fix off-by-one in ipip6_tunnel_get

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Mon, 12 Oct 2009 16:58:14 -0700
From:	"Templin, Fred L" <Fred.L.Templin@...ing.com>
To:	Wolfgang Walter <wolfgang.walter@...m.de>,
	David Miller <davem@...emloft.net>
CC:	"gregkh@...e.de" <gregkh@...e.de>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"stable@...nel.org" <stable@...nel.org>,
	"stable-review@...nel.org" <stable-review@...nel.org>,
	"torvalds@...ux-foundation.org" <torvalds@...ux-foundation.org>,
	"akpm@...ux-foundation.org" <akpm@...ux-foundation.org>,
	"alan@...rguk.ukuu.org.uk" <alan@...rguk.ukuu.org.uk>,
	"contact@...chahlusiak.de" <contact@...chahlusiak.de>,
	"yoshfuji@...ux-ipv6.org" <yoshfuji@...ux-ipv6.org>
Subject: RE: [patch 37/37] sit: fix off-by-one in ipip6_tunnel_get_prl

Wolfgang,

Thanks for your comments. As I wrote to David and Greg,
I tested the patch and it is indeed good. I'd really
rather not get into changing other code that has
already been tested at this time, however.

Regards - Fred
fred.l.templin@...ing.com

> -----Original Message-----
> From: Wolfgang Walter [mailto:wolfgang.walter@...m.de]
> Sent: Saturday, October 10, 2009 6:30 PM
> To: David Miller
> Cc: Templin, Fred L; gregkh@...e.de; linux-kernel@...r.kernel.org; stable@...nel.org; stable-
> review@...nel.org; torvalds@...ux-foundation.org; akpm@...ux-foundation.org;
> alan@...rguk.ukuu.org.uk; contact@...chahlusiak.de; yoshfuji@...ux-ipv6.org
> Subject: Re: [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" <Fred.L.Templin@...ing.com>
> > 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
> prl.
> 
> 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:
> 
> 1)
> 
> 	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:
> 
> 	out:
> 		read_unlock(&ipip6_lock);
> 
> 		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
> modifiied.
> 
> And either
> 
> 	if (! ret) {
> 		len = sizeof(*kp) * c;
> 		if ((len && copy_to_user(a + 1, kp, len)) || put_user(len, &a->datalen))
> 			ret = -EFAULT;
> 	}
> 
> or
> 
> 	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.
> 
> 
> Regards,
> --
> 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 majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/