| 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
| ||
|
Message-Id: <201001051620.38943.arnd@arndb.de>
Date: Tue, 5 Jan 2010 16:20:38 +0100
From: Arnd Bergmann <arnd@...db.de>
To: Arjan van de Ven <arjan@...radead.org>
Cc: Heiko Carstens <heiko.carstens@...ibm.com>,
Ingo Molnar <mingo@...e.hu>,
David Miller <davem@...emloft.net>,
Andrew Morton <akpm@...ux-foundation.org>,
linux-kernel@...r.kernel.org
Subject: Re: strict copy_from_user checks issues?
On Tuesday 05 January 2010, Arjan van de Ven wrote:
> On Tue, 5 Jan 2010 14:45:25 +0100
> Arnd Bergmann <arnd@...db.de> wrote:
> >
> > I think it will get inlined on 32 bit machines or without
> > CONFIG_COMPAT, but not when CONFIG_COMPAT is enabled, because then
> > there are two call-sites.
>
> one of them is buggy it seems;
> it passes in a shorter length, but there is no code in sight that makes
> sure that the end of the structure (the difference between the shorter
> and full length one) gets initialized to, say, zeros rather than stack
> garbage. So looks like there is at least a bug there.
The old code (until 2.6.32) always copied sizeof (struct ifreq), which
I changed in order not corrupt the user space stack.
I checked that no code in the tun driver accesses the incompatible
parts of the structure, which would require conversion rather
than simply doing a short read, so it looks correct to me, or am I
missing something?
> Would be nice if the copy (+ clear) would be pulled to the two callers
> I suspect... at which point the warning will go away too as a side
> effect.
You mean like this?
It adds some complexity and about 200 bytes of object code,
I'm not sure it's worth it.
--
[UNTESTED PATCH] tun: avoid copy_from_user size warning
For 32 bit compat code, the tun driver only copies the
fields it needs using a short length, which copy_from_user
now warns about. Moving the copy operation outside of the
main ioctl function lets us avoid the warning.
Signed-off-by: Arnd Bergmann <arnd@...db.de>
---
drivers/net/tun.c | 104 +++++++++++++++++++++++++++++++----------------------
1 files changed, 61 insertions(+), 43 deletions(-)
diff --git a/drivers/net/tun.c b/drivers/net/tun.c
index 01e99f2..8eb9f38 100644
--- a/drivers/net/tun.c
+++ b/drivers/net/tun.c
@@ -1111,19 +1111,14 @@ static int set_offload(struct net_device *dev, unsigned long arg)
}
static long __tun_chr_ioctl(struct file *file, unsigned int cmd,
- unsigned long arg, int ifreq_len)
+ unsigned long arg, struct ifreq *ifr)
{
struct tun_file *tfile = file->private_data;
struct tun_struct *tun;
void __user* argp = (void __user*)arg;
- struct ifreq ifr;
int sndbuf;
int ret;
- if (cmd == TUNSETIFF || _IOC_TYPE(cmd) == 0x89)
- if (copy_from_user(&ifr, argp, ifreq_len))
- return -EFAULT;
-
if (cmd == TUNGETFEATURES) {
/* Currently this just means: "what IFF flags are valid?".
* This is needed because we never checked for invalid flags on
@@ -1136,34 +1131,21 @@ static long __tun_chr_ioctl(struct file *file, unsigned int cmd,
rtnl_lock();
tun = __tun_get(tfile);
- if (cmd == TUNSETIFF && !tun) {
- ifr.ifr_name[IFNAMSIZ-1] = '\0';
-
- ret = tun_set_iff(tfile->net, file, &ifr);
-
- if (ret)
- goto unlock;
-
- if (copy_to_user(argp, &ifr, ifreq_len))
- ret = -EFAULT;
+ if (!tun) {
+ ret = -EBADFD;
+ if (cmd == TUNSETIFF) {
+ ifr->ifr_name[IFNAMSIZ-1] = '\0';
+ ret = tun_set_iff(tfile->net, file, ifr);
+ }
goto unlock;
}
- ret = -EBADFD;
- if (!tun)
- goto unlock;
-
DBG(KERN_INFO "%s: tun_chr_ioctl cmd %d\n", tun->dev->name, cmd);
ret = 0;
switch (cmd) {
case TUNGETIFF:
- ret = tun_get_iff(current->nsproxy->net_ns, tun, &ifr);
- if (ret)
- break;
-
- if (copy_to_user(argp, &ifr, ifreq_len))
- ret = -EFAULT;
+ ret = tun_get_iff(current->nsproxy->net_ns, tun, ifr);
break;
case TUNSETNOCSUM:
@@ -1234,18 +1216,16 @@ static long __tun_chr_ioctl(struct file *file, unsigned int cmd,
case SIOCGIFHWADDR:
/* Get hw addres */
- memcpy(ifr.ifr_hwaddr.sa_data, tun->dev->dev_addr, ETH_ALEN);
- ifr.ifr_hwaddr.sa_family = tun->dev->type;
- if (copy_to_user(argp, &ifr, ifreq_len))
- ret = -EFAULT;
+ memcpy(ifr->ifr_hwaddr.sa_data, tun->dev->dev_addr, ETH_ALEN);
+ ifr->ifr_hwaddr.sa_family = tun->dev->type;
break;
case SIOCSIFHWADDR:
/* Set hw address */
DBG(KERN_DEBUG "%s: set hw address: %pM\n",
- tun->dev->name, ifr.ifr_hwaddr.sa_data);
+ tun->dev->name, ifr->ifr_hwaddr.sa_data);
- ret = dev_set_mac_address(tun->dev, &ifr.ifr_hwaddr);
+ ret = dev_set_mac_address(tun->dev, &ifr->ifr_hwaddr);
break;
case TUNGETSNDBUF:
@@ -1278,35 +1258,73 @@ unlock:
static long tun_chr_ioctl(struct file *file,
unsigned int cmd, unsigned long arg)
{
- return __tun_chr_ioctl(file, cmd, arg, sizeof (struct ifreq));
+ struct ifreq ifr;
+ struct ifreq __user *uifr = (void *)arg;
+ int ret;
+
+ switch (cmd) {
+ case TUNSETIFF:
+ case TUNGETIFF:
+ case SIOCGIFHWADDR:
+ case SIOCSIFHWADDR:
+ if (copy_from_user(&ifr, uifr, sizeof (ifr)))
+ return -EFAULT;
+
+ ret = __tun_chr_ioctl(file, cmd, arg, &ifr);
+ if (ret < 0)
+ return ret;
+
+ if (copy_to_user(uifr, &ifr, sizeof (ifr)))
+ return -EFAULT;
+
+ return ret;
+ }
+
+ return __tun_chr_ioctl(file, cmd, arg, NULL);
}
#ifdef CONFIG_COMPAT
static long tun_chr_compat_ioctl(struct file *file,
unsigned int cmd, unsigned long arg)
{
+ struct ifreq ifr;
+ struct compat_ifreq __user *uifr = compat_ptr(arg);
+ int ret;
+
switch (cmd) {
case TUNSETIFF:
case TUNGETIFF:
+ case SIOCGIFHWADDR:
+ case SIOCSIFHWADDR:
+ /*
+ * compat_ifreq is shorter than ifreq, so we must not access beyond
+ * the end of that structure. All fields that are used in this
+ * driver are compatible though, we don't need to convert the
+ * contents.
+ */
+ memset(&ifr, 0, sizeof (ifr));
+ if (copy_from_user((struct compat_ifreq *)&ifr, uifr, sizeof (*uifr)))
+ return -EFAULT;
+
+ ret = __tun_chr_ioctl(file, cmd, 0, &ifr);
+ if (ret)
+ return ret;
+
+ if (copy_to_user(uifr, (struct compat_ifreq *)&ifr, sizeof (*uifr)))
+ return -EFAULT;
+ return ret;
+ break;
+
case TUNSETTXFILTER:
case TUNGETSNDBUF:
case TUNSETSNDBUF:
- case SIOCGIFHWADDR:
- case SIOCSIFHWADDR:
arg = (unsigned long)compat_ptr(arg);
break;
default:
arg = (compat_ulong_t)arg;
break;
}
-
- /*
- * compat_ifreq is shorter than ifreq, so we must not access beyond
- * the end of that structure. All fields that are used in this
- * driver are compatible though, we don't need to convert the
- * contents.
- */
- return __tun_chr_ioctl(file, cmd, arg, sizeof(struct compat_ifreq));
+ return __tun_chr_ioctl(file, cmd, arg, NULL);
}
#endif /* CONFIG_COMPAT */
--
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/
Powered by blists - more mailing lists