| Message ID | 1249139060-15392-4-git-send-email-hmh@hmh.eng.br (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
On Saturday 01 August 2009 17:04:19 Henrique de Moraes Holschuh wrote: > From: Michael Buesch <mb@bu3sch.de> > > Avoid a heap buffer overrun triggered by an integer overflow of the > userspace controlled "count" variable. > > If userspace passes in a "count" of (size_t)-1l, the kmalloc size will > overflow to ((size_t)-1l + 2) = 1, so only one byte will be allocated. > However, copy_from_user() will attempt to copy 0xFFFFFFFF (or > 0xFFFFFFFFFFFFFFFF on 64bit) bytes to the buffer. > > A possible testcase could look like this: > > #include <stdio.h> > #include <stdlib.h> > #include <unistd.h> > #include <fcntl.h> > > int main(int argc, char **argv) > { > int fd; > char c; > > if (argc != 2) { > printf("Usage: %s /proc/acpi/ibm/filename\n", argv[0]); > return 1; > } > fd = open(argv[1], O_RDWR); > if (fd < 0) { > printf("Could not open proc file\n"); > return 1; > } > write(fd, &c, (size_t)-1l); > } > > We avoid the integer overrun by putting an arbitrary limit on the count. > PAGE_SIZE sounds like a sane limit. > > (note: this bug exists at least since kernel 2.6.12...) > > Signed-off-by: Michael Buesch <mb@bu3sch.de> > Acked-by: Henrique de Moraes Holschuh <hmh@hmh.eng.br> > Cc: stable@kernel.org > --- > drivers/platform/x86/thinkpad_acpi.c | 2 ++ > 1 files changed, 2 insertions(+), 0 deletions(-) > > diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c > index 27d68e7..18f9ee6 100644 > --- a/drivers/platform/x86/thinkpad_acpi.c > +++ b/drivers/platform/x86/thinkpad_acpi.c > @@ -766,6 +766,8 @@ static int dispatch_procfs_write(struct file *file, > > if (!ibm || !ibm->write) > return -EINVAL; > + if (count > PAGE_SIZE - 2) > + return -EINVAL; > > kernbuf = kmalloc(count + 2, GFP_KERNEL); > if (!kernbuf) Note that it turns out this is not a real-life bug after all. The VFS code checks count for signedness (high bit set) and bails out if this is the case. Well, it might probably be a good idea to restrict the count range to something sane here anyway, so...
On Sat, 01 Aug 2009, Michael Buesch wrote: > On Saturday 01 August 2009 17:04:19 Henrique de Moraes Holschuh wrote: > > From: Michael Buesch <mb@bu3sch.de> > > > > Avoid a heap buffer overrun triggered by an integer overflow of the > > userspace controlled "count" variable. > > > > If userspace passes in a "count" of (size_t)-1l, the kmalloc size will > > overflow to ((size_t)-1l + 2) = 1, so only one byte will be allocated. > > However, copy_from_user() will attempt to copy 0xFFFFFFFF (or > > 0xFFFFFFFFFFFFFFFF on 64bit) bytes to the buffer. > > > > A possible testcase could look like this: > > > > #include <stdio.h> > > #include <stdlib.h> > > #include <unistd.h> > > #include <fcntl.h> > > > > int main(int argc, char **argv) > > { > > int fd; > > char c; > > > > if (argc != 2) { > > printf("Usage: %s /proc/acpi/ibm/filename\n", argv[0]); > > return 1; > > } > > fd = open(argv[1], O_RDWR); > > if (fd < 0) { > > printf("Could not open proc file\n"); > > return 1; > > } > > write(fd, &c, (size_t)-1l); > > } > > > > We avoid the integer overrun by putting an arbitrary limit on the count. > > PAGE_SIZE sounds like a sane limit. > > > > (note: this bug exists at least since kernel 2.6.12...) > > > > Signed-off-by: Michael Buesch <mb@bu3sch.de> > > Acked-by: Henrique de Moraes Holschuh <hmh@hmh.eng.br> > > Cc: stable@kernel.org > > --- > > drivers/platform/x86/thinkpad_acpi.c | 2 ++ > > 1 files changed, 2 insertions(+), 0 deletions(-) > > > > diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c > > index 27d68e7..18f9ee6 100644 > > --- a/drivers/platform/x86/thinkpad_acpi.c > > +++ b/drivers/platform/x86/thinkpad_acpi.c > > @@ -766,6 +766,8 @@ static int dispatch_procfs_write(struct file *file, > > > > if (!ibm || !ibm->write) > > return -EINVAL; > > + if (count > PAGE_SIZE - 2) > > + return -EINVAL; > > > > kernbuf = kmalloc(count + 2, GFP_KERNEL); > > if (!kernbuf) > > Note that it turns out this is not a real-life bug after all. > The VFS code checks count for signedness (high bit set) and bails > out if this is the case. > Well, it might probably be a good idea to restrict the count range to > something sane here anyway, so... It is a good idea to restrict the maximum size to something sane anyway. But the commit message needs to be fixed if there is no security hole. Anyway, in which function and file is the VFS code that restricts the maximum size?
applied w/ simplified check-in commment thanks, Len Brown, Intel Open Source Technology Center -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sunday 02 August 2009 03:50:12 Henrique de Moraes Holschuh wrote: > > Note that it turns out this is not a real-life bug after all. > > The VFS code checks count for signedness (high bit set) and bails > > out if this is the case. > > Well, it might probably be a good idea to restrict the count range to > > something sane here anyway, so... > > It is a good idea to restrict the maximum size to something sane anyway. > > But the commit message needs to be fixed if there is no security hole. > > Anyway, in which function and file is the VFS code that restricts the > maximum size? > Well there are two things. It happens that access_ok() on x86 does such a check. It does _not_ do this on all arches, but as a thinkpad is x86... And there are sanity checks in fs/read_write.c: 208 /* 209 * rw_verify_area doesn't like huge counts. We limit 210 * them to something that fits in "int" so that others 211 * won't have to do range checks all the time. 212 */ 213 #define MAX_RW_COUNT (INT_MAX & PAGE_CACHE_MASK) 214 215 int rw_verify_area(int read_write, struct file *file, loff_t *ppos, size_t count) 216 { 217 struct inode *inode; 218 loff_t pos; 219 int retval = -EINVAL; 220 221 inode = file->f_path.dentry->d_inode; 222 if (unlikely((ssize_t) count < 0)) 223 return retval; 224 pos = *ppos; 225 if (unlikely((pos < 0) || (loff_t) (pos + count) < 0)) 226 return retval; 227 228 if (unlikely(inode->i_flock && mandatory_lock(inode))) { 229 retval = locks_mandatory_area( 230 read_write == READ ? FLOCK_VERIFY_READ : FLOCK_VERIFY_WRITE, 231 inode, file, pos, count); 232 if (retval < 0) 233 return retval; 234 } 235 retval = security_file_permission(file, 236 read_write == READ ? MAY_READ : MAY_WRITE); 237 if (retval) 238 return retval; 239 return count > MAX_RW_COUNT ? MAX_RW_COUNT : count; 240 }
On Sunday 02 August 2009 06:11:13 Len Brown wrote: > applied w/ simplified check-in commment > > thanks, > Len Brown, Intel Open Source Technology Center Thanks. The same discussion applies to the toshiba_acpi patch I sent to you.
diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c index 27d68e7..18f9ee6 100644 --- a/drivers/platform/x86/thinkpad_acpi.c +++ b/drivers/platform/x86/thinkpad_acpi.c @@ -766,6 +766,8 @@ static int dispatch_procfs_write(struct file *file, if (!ibm || !ibm->write) return -EINVAL; + if (count > PAGE_SIZE - 2) + return -EINVAL; kernbuf = kmalloc(count + 2, GFP_KERNEL); if (!kernbuf)