drm: udl: Properly check framebuffer mmap offsets

PATCH Applied Greg Kroah-Hartman 3 participants 6 messages Mar 21, 2018 – Mar 26, 2018

Subsystems (3)

DRM drivers DRM misc DRM driver for USB displaylink video adapters

lore

Share & tools

lore

Greg Kroah-Hartman

author

2018-03-21 15:45 UTC #0 lore raw

PATCH 1/1 drm: udl: Properly check framebuffer mmap offsets

The memmap options sent to the udl framebuffer driver were not being checked for all sets of possible crazy values. Fix this up by properly bounding the allowed values.

Reported-by Eyal Itkin <[email protected]>

Cc stable <[email protected]>

Signed-off-by Greg Kroah-Hartman <[email protected]>

drivers/gpu/drm/udl/udl_fb.c +7 -2

index b5b335c9b2bb..2ebdc6d5a76e 100644
@@ -159,10 +159,15 @@ static int udl_fb_mmap(struct fb_info *info, struct vm_area_struct *vma)
 {
 	unsigned long start = vma->vm_start;
 	unsigned long size = vma->vm_end - vma->vm_start;
-	unsigned long offset = vma->vm_pgoff << PAGE_SHIFT;
+	unsigned long offset;
 	unsigned long page, pos;
 
-	if (offset + size > info->fix.smem_len)
+	if (vma->vm_pgoff > (~0UL >> PAGE_SHIFT))
+		return -EINVAL;
+
+	offset = vma->vm_pgoff << PAGE_SHIFT;
+
+	if (offset > info->fix.smem_len || size > info->fix.smem_len - offset)
 		return -EINVAL;
 
 	pos = (unsigned long)info->fix.smem_start + offset;

_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Daniel Vetter

Applied

2018-03-22 07:00 UTC #1 lore raw

Quoted 7 lines · Greg Kroah-Hartman

The memmap options sent to the udl framebuffer driver were not being checked for all sets of possible crazy values. Fix this up by properly bounding the allowed values.
Reported-by Eyal Itkin <[email protected]>
Cc stable <[email protected]>
Signed-off-by Greg Kroah-Hartman <[email protected]>

Applied to drm-misc-fixes, thanks for the patch.

Does anyone working on overflow-proof integers? That would make a lot of this code so much simpler if we could just ask the compiler to carry the oferflow bit around for a given expression and then check that and bail with -EINVAL.
-Daniel

Quoted 23 lines

drivers/gpu/drm/udl/udl_fb.c +7 -2

index b5b335c9b2bb..2ebdc6d5a76e 100644
@@ -159,10 +159,15 @@ static int udl_fb_mmap(struct fb_info *info, struct vm_area_struct *vma)
 {
 	unsigned long start = vma->vm_start;
 	unsigned long size = vma->vm_end - vma->vm_start;
-	unsigned long offset = vma->vm_pgoff << PAGE_SHIFT;
+	unsigned long offset;
 	unsigned long page, pos;
 
-	if (offset + size > info->fix.smem_len)
+	if (vma->vm_pgoff > (~0UL >> PAGE_SHIFT))
+		return -EINVAL;
+
+	offset = vma->vm_pgoff << PAGE_SHIFT;
+
+	if (offset > info->fix.smem_len || size > info->fix.smem_len - offset)
 		return -EINVAL;
 
 	pos = (unsigned long)info->fix.smem_start + offset;

Signature · 7 lines

Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Greg Kroah-Hartman

author

2018-03-22 08:03 UTC #2 lore raw

Quoted 4 lines · Daniel Vetter

Does anyone working on overflow-proof integers? That would make a lot of this code so much simpler if we could just ask the compiler to carry the oferflow bit around for a given expression and then check that and bail with -EINVAL.

That would be nice, but no, I don't think that's part of any C standard work that I have heard of :(

greg k-h
_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Daniel Vetter

2018-03-22 09:54 UTC #3 lore raw

Quoted 8 lines · Greg Kroah-Hartman

Quoted 4 lines · Daniel Vetter
Does anyone working on overflow-proof integers? That would make a lot of this code so much simpler if we could just ask the compiler to carry the oferflow bit around for a given expression and then check that and bail with -EINVAL.
That would be nice, but no, I don't think that's part of any C standard work that I have heard of :(

Well we have refcount_t already, stitching something together that would work and not suck too badly with performance should be possible. But yeah direct compiler support would be better (and would allow optimizing the carry flag checks I guess). I kinda hoped Kees&team would be working on this eventually.

Adding Kees+kernel-hardening, maybe he'll grow fond of this :-) -Daniel

Signature · 3 lines

Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Kees Cook

2018-03-23 00:14 UTC #4 lore raw

Quoted 8 lines · Daniel Vetter

Quoted 6 lines · Greg Kroah-Hartman
Quoted 4 lines · Daniel Vetter
Does anyone working on overflow-proof integers? That would make a lot of this code so much simpler if we could just ask the compiler to carry the oferflow bit around for a given expression and then check that and bail with -EINVAL.

Quoted 8 lines

Quoted 2 lines
That would be nice, but no, I don't think that's part of any C standard work that I have heard of :(
Well we have refcount_t already, stitching something together that would work and not suck too badly with performance should be possible. But yeah direct compiler support would be better (and would allow optimizing the carry flag checks I guess). I kinda hoped Kees&team would be working on this eventually.

refcount_t could be used if it happens to match the needed semantics.

Quoted 1 line

Adding Kees+kernel-hardening, maybe he'll grow fond of this :-)

Yeah, general integer overflow is on the list of things to get fixed in the kernel. It's a bit of a long road, though.

Clang has -fsanitize=integer (and sub-options) which could be added for specific object or trees:
https://clang.llvm.org/docs/UndefinedBehaviorSanitizer.html#ubsan-checks

GCC seems to only support manual marking of overflow detections:
https://gcc.gnu.org/onlinedocs/gcc/Integer-Overflow-Builtins.html

grsecurity/PaX has a gcc plugin for overflow detection, though it hasn't been upstreamed and comes with various caveats:
http://forums.grsecurity.net/viewtopic.php?f=7&t=3043
https://github.com/ephox-gcc-plugins/size_overflow

-Kees

Signature · 2 lines

Kees Cook
Pixel Security

Daniel Vetter

2018-03-26 08:26 UTC #5 lore raw

Quoted 20 lines · Kees Cook

Quoted 8 lines · Daniel Vetter
Quoted 6 lines · Greg Kroah-Hartman
Quoted 4 lines · Daniel Vetter
Does anyone working on overflow-proof integers? That would make a lot of this code so much simpler if we could just ask the compiler to carry the oferflow bit around for a given expression and then check that and bail with -EINVAL.
Quoted 8 lines
Quoted 2 lines
That would be nice, but no, I don't think that's part of any C standard work that I have heard of :(
Well we have refcount_t already, stitching something together that would work and not suck too badly with performance should be possible. But yeah direct compiler support would be better (and would allow optimizing the carry flag checks I guess). I kinda hoped Kees&team would be working on this eventually.
refcount_t could be used if it happens to match the needed semantics.

Quoted 8 lines

Quoted 1 line
Adding Kees+kernel-hardening, maybe he'll grow fond of this :-)
Yeah, general integer overflow is on the list of things to get fixed in the kernel. It's a bit of a long road, though.
Clang has -fsanitize=integer (and sub-options) which could be added for specific object or trees:
https://clang.llvm.org/docs/UndefinedBehaviorSanitizer.html#ubsan-checks

I expect that a bunch of folks will scream about checking all interger math for overflows

Quoted 2 lines

GCC seems to only support manual marking of overflow detections:
https://gcc.gnu.org/onlinedocs/gcc/Integer-Overflow-Builtins.html

And this sounds _very_ verbose. What I have in mind is something like

	result = overflow_checked(expr, &overflow)
	if (overflow)
		return -EINVAL;

and expr could be any expression at all (except calling functions ofc, or whatever the correct verbiage from the C standard for this is). I don't want to annotate each compuation individually at least, that's too much typing :-) Even neater would be an entire block:

	while {
		/* compute stuff except function calls, but allow inlining */
	} has_overflow {
		return -EINVAL;
	}

Anyway, yay for this being somewhere on the future plans!

Quoted 4 lines

grsecurity/PaX has a gcc plugin for overflow detection, though it hasn't been upstreamed and comes with various caveats:
http://forums.grsecurity.net/viewtopic.php?f=7&t=3043
https://github.com/ephox-gcc-plugins/size_overflow

Yeah this looks fairly fragile to me, also can't use the overflow bits instructions sets have I think.
-Daniel

Signature · 3 lines

Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch