Videobuf2 corner case fixes
Subsystems (2)
v2 cover Videobuf2 corner case fixes
Hi all,
Here's a second version of the set fixing a few videobuf2 corner cases. Most drivers have limits for the size already but not necessarily all of them.
since v1:
- Add a sanity check for alignment in vb2_dma_sg_alloc_compacted.
- Add a comment in __vb2_buf_mem_alloc noting that the size shall be page aligned.
Sakari Ailus (3):
videobuf2-core: Prevent size alignment wrapping buffer size to 0
videobuf2-dma-sg: Prevent size from overflowing videobuf2-core.h: Document the alloc memop size argument as page
aligned
drivers/media/common/videobuf2/videobuf2-core.c | 5 +++++ drivers/media/common/videobuf2/videobuf2-dma-sg.c | 5 ++++- include/media/videobuf2-core.h | 3 ++- 3 files changed, 11 insertions(+), 2 deletions(-)
Signature · 1 line
2.11.0
PATCH v2 1/3 videobuf2-core: Prevent size alignment wrapping buffer size to 0
PAGE_ALIGN() may wrap the buffer size around to 0. Prevent this by checking that the aligned value is not smaller than the unaligned one.
Note on backporting to stable: the file used to be under drivers/media/v4l2-core, it was moved to the current location after 4.14.
--- drivers/media/common/videobuf2/videobuf2-core.c | 4 ++++ 1 file changed, 4 insertions(+)
drivers/media/common/videobuf2/videobuf2-core.c +4 -0
@@ -207,6 +207,10 @@ static int __vb2_buf_mem_alloc(struct vb2_buffer *vb)for (plane =0 ; plane < vb->num_planes; ++plane) {unsigned long size =PAGE_ALIGN (vb->planes[plane].length); +/* Did it wrap around? */ +if (size < vb->planes[plane].length) + goto free; + mem_priv =call_ptr_memop (vb, alloc, q->alloc_devs[plane] ? : q->dev, q->dma_attrs, size, q->dma_dir, q->gfp_flags);
Signature · 1 line
2.11.0
Mauro Carvalho Chehab
maintainerRe: [PATCH v2 1/3] videobuf2-core: Prevent size alignment wrapping buffer size to 0
Em Tue, 8 Jan 2019 10:58:34 +0200
Sakari Ailus <[email protected]> escreveu:
Quoted 18 lines
PAGE_ALIGN()may wrap the buffer size around to 0. Prevent this by checking that the aligned value is not smaller than the unaligned one.Note on backporting to stable: the file used to be under
drivers/media/v4l2-core, it was moved to the current location after 4.14.Signed-off-by Sakari Ailus <[email protected]>Reviewed-by Hans Verkuil <[email protected]>--- drivers/media/common/videobuf2/videobuf2-core.c | 4 ++++ 1 file changed, 4 insertions(+)drivers/media/common/videobuf2/videobuf2-core.c +0
-0
drivers/media/common/videobuf2/videobuf2-core.c static int __vb2_buf_mem_alloc(struct vb2_buffer *vb) · line 213
Quoted 7 lines
drivers/media/common/videobuf2/videobuf2-core.c +4
-0@@ -207,6 +207,10 @@ static int __vb2_buf_mem_alloc(struct vb2_buffer *vb)for (plane =0 ; plane < vb->num_planes; ++plane) {unsigned long size =PAGE_ALIGN (vb->planes[plane].length); +/* Did it wrap around? */ +if (size < vb->planes[plane].length) + goto free; +
Sorry, but I can't see how this could ever happen (except for a very serious bug at the compiler or at the hardware).
See, the definition at PAGE_ALIGN is (from mm.h):
#define PAGE_ALIGN(addr) ALIGN(addr, PAGE_SIZE)
and the macro it uses come from kernel.h:
#define __ALIGN_KERNEL(x, a) __ALIGN_KERNEL_MASK(x, (typeof(x))(a) - 1) #define __ALIGN_KERNEL_MASK(x, mask) (((x) + (mask)) & ~(mask)) .. #define ALIGN(x, a) __ALIGN_KERNEL((x), (a))
So, this:
size = PAGE_ALIGN(length);
(assuming PAGE_SIZE= 0x1000)
becomes:
size = (length + 0x0fff) & ~0xfff;
so, size will *always* be >= length.
Thanks,
Mauro
Re: [PATCH v2 1/3] videobuf2-core: Prevent size alignment wrapping buffer size to 0
Em Tue, 8 Jan 2019 10:52:12 -0200
Mauro Carvalho Chehab <[email protected]> escreveu:
Quoted 46 lines
Em Tue, 8 Jan 2019 10:58:34 +0200
Sakari Ailus <[email protected]> escreveu:Quoted 18 lines
PAGE_ALIGN()may wrap the buffer size around to 0. Prevent this by checking that the aligned value is not smaller than the unaligned one.Note on backporting to stable: the file used to be under
drivers/media/v4l2-core, it was moved to the current location after 4.14.Signed-off-by Sakari Ailus <[email protected]>Reviewed-by Hans Verkuil <[email protected]>--- drivers/media/common/videobuf2/videobuf2-core.c | 4 ++++ 1 file changed, 4 insertions(+)drivers/media/common/videobuf2/videobuf2-core.c +0
-0drivers/media/common/videobuf2/videobuf2-core.c static int __vb2_buf_mem_alloc(struct vb2_buffer *vb) · line 213
Quoted 7 lines
drivers/media/common/videobuf2/videobuf2-core.c +4
-0@@ -207,6 +207,10 @@ static int __vb2_buf_mem_alloc(struct vb2_buffer *vb)for (plane =0 ; plane < vb->num_planes; ++plane) {unsigned long size =PAGE_ALIGN (vb->planes[plane].length); +/* Did it wrap around? */ +if (size < vb->planes[plane].length) + goto free; +Sorry, but I can't see how this could ever happen (except for a very serious bug at the compiler or at the hardware).
See, the definition at
PAGE_ALIGNis (frommm.h):#define PAGE_ALIGN(addr) ALIGN(addr, PAGE_SIZE)and the macro it uses come from
kernel.h:#define __ALIGN_KERNEL(x, a) __ALIGN_KERNEL_MASK(x, (typeof(x))(a) - 1) #define __ALIGN_KERNEL_MASK(x, mask) (((x) + (mask)) & ~(mask)) .. #define ALIGN(x, a) __ALIGN_KERNEL((x), (a))So, this:
size = PAGE_ALIGN(length);
Quoted 7 lines
(assuming PAGE_SIZE=
0x1000)becomes:
size = (length + 0x0fff) & ~0xfff;so, size will *always* be >= length.
Hmm... after looking at patch 2, now I understand what's your concern...
If someone indeed uses length = INT_MAX, size will indeed be zero.
Please adjust the description accordingly, as it doesn't reflect that.
Btw, in this particular case, I would use a WARN_ON(), as this is something that indicates not only a driver bug (as the driver is letting someone to request a buffer a way too big), but probably also an attempt from a hacker to try to crack the system.
Thanks,
Mauro
Re: [PATCH v2 1/3] videobuf2-core: Prevent size alignment wrapping buffer size to 0
Em Tue, 8 Jan 2019 10:59:55 -0200
Mauro Carvalho Chehab <[email protected]> escreveu:
Quoted 53 lines
Em Tue, 8 Jan 2019 10:52:12 -0200
Mauro Carvalho Chehab <[email protected]> escreveu:Quoted 50 lines
Em Tue, 8 Jan 2019 10:58:34 +0200
Sakari Ailus <[email protected]> escreveu:Quoted 18 lines
PAGE_ALIGN()may wrap the buffer size around to 0. Prevent this by checking that the aligned value is not smaller than the unaligned one.Note on backporting to stable: the file used to be under
drivers/media/v4l2-core, it was moved to the current location after 4.14.Signed-off-by Sakari Ailus <[email protected]>Reviewed-by Hans Verkuil <[email protected]>--- drivers/media/common/videobuf2/videobuf2-core.c | 4 ++++ 1 file changed, 4 insertions(+)drivers/media/common/videobuf2/videobuf2-core.c +0
-0drivers/media/common/videobuf2/videobuf2-core.c static int __vb2_buf_mem_alloc(struct vb2_buffer *vb) · line 213
Quoted 7 lines
drivers/media/common/videobuf2/videobuf2-core.c +4
-0@@ -207,6 +207,10 @@ static int __vb2_buf_mem_alloc(struct vb2_buffer *vb)for (plane =0 ; plane < vb->num_planes; ++plane) {unsigned long size =PAGE_ALIGN (vb->planes[plane].length); +/* Did it wrap around? */ +if (size < vb->planes[plane].length) + goto free; +Sorry, but I can't see how this could ever happen (except for a very serious bug at the compiler or at the hardware).
See, the definition at
PAGE_ALIGNis (frommm.h):#define PAGE_ALIGN(addr) ALIGN(addr, PAGE_SIZE)and the macro it uses come from
kernel.h:#define __ALIGN_KERNEL(x, a) __ALIGN_KERNEL_MASK(x, (typeof(x))(a) - 1) #define __ALIGN_KERNEL_MASK(x, mask) (((x) + (mask)) & ~(mask)) .. #define ALIGN(x, a) __ALIGN_KERNEL((x), (a))So, this:
size = PAGE_ALIGN(length);(assuming PAGE_SIZE=
0x1000)becomes:
Quoted 7 lines
Quoted 3 lines
size = (length + 0x0fff) & ~0xfff;so, size will *always* be >= length.
Hmm... after looking at patch 2, now I understand what's your concern...
If someone indeed uses length =
INT_MAX, size will indeed be zero.
In time, I meant to say UINT_MAX.
Quoted 11 lines
Please adjust the description accordingly, as it doesn't reflect that.
Btw, in this particular case, I would use a
WARN_ON(), as this is something that indicates not only a driver bug (as the driver is letting someone to request a buffer a way too big), but probably also an attempt from a hacker to try to crack the system.Thanks,
Mauro
Thanks,
Mauro
Re: [PATCH v2 1/3] videobuf2-core: Prevent size alignment wrapping buffer size to 0
In reply to Mauro Carvalho ChehabHi Mauro,
Thanks for the review.
Quoted 59 lines · Mauro Carvalho Chehab
Em Tue, 8 Jan 2019 10:52:12 -0200
Mauro Carvalho Chehab <[email protected]> escreveu:Quoted 46 lines
Em Tue, 8 Jan 2019 10:58:34 +0200
Sakari Ailus <[email protected]> escreveu:Quoted 18 lines
PAGE_ALIGN()may wrap the buffer size around to 0. Prevent this by checking that the aligned value is not smaller than the unaligned one.Note on backporting to stable: the file used to be under
drivers/media/v4l2-core, it was moved to the current location after 4.14.Signed-off-by Sakari Ailus <[email protected]>Reviewed-by Hans Verkuil <[email protected]>--- drivers/media/common/videobuf2/videobuf2-core.c | 4 ++++ 1 file changed, 4 insertions(+)drivers/media/common/videobuf2/videobuf2-core.c +0
-0drivers/media/common/videobuf2/videobuf2-core.c static int __vb2_buf_mem_alloc(struct vb2_buffer *vb) · line 213
Quoted 7 lines
drivers/media/common/videobuf2/videobuf2-core.c +4
-0@@ -207,6 +207,10 @@ static int __vb2_buf_mem_alloc(struct vb2_buffer *vb)for (plane =0 ; plane < vb->num_planes; ++plane) {unsigned long size =PAGE_ALIGN (vb->planes[plane].length); +/* Did it wrap around? */ +if (size < vb->planes[plane].length) + goto free; +Sorry, but I can't see how this could ever happen (except for a very serious bug at the compiler or at the hardware).
See, the definition at
PAGE_ALIGNis (frommm.h):#define PAGE_ALIGN(addr) ALIGN(addr, PAGE_SIZE)and the macro it uses come from
kernel.h:#define __ALIGN_KERNEL(x, a) __ALIGN_KERNEL_MASK(x, (typeof(x))(a) - 1) #define __ALIGN_KERNEL_MASK(x, mask) (((x) + (mask)) & ~(mask)) .. #define ALIGN(x, a) __ALIGN_KERNEL((x), (a))So, this:
size = PAGE_ALIGN(length);Quoted 7 lines
(assuming PAGE_SIZE=
0x1000)becomes:
size = (length + 0x0fff) & ~0xfff;so, size will *always* be >= length.
Hmm... after looking at patch 2, now I understand what's your concern...
Quoted 8 lines
If someone indeed uses length =
INT_MAX, size will indeed be zero.Please adjust the description accordingly, as it doesn't reflect that.
Btw, in this particular case, I would use a
WARN_ON(), as this is something that indicates not only a driver bug (as the driver is letting someone to request a buffer a way too big), but probably
What's the maximum size a driver should allow? I guess this could be seen be a failure from the driver's part to limit the size of the buffer, but it's not trivial either to define that.
Hardware typically has maximum dimensions it can support, but the user may want to add padding at the end of the lines. Perhaps a helper macro could be used for this purpose: most likely there's no need to be more padding than there's image data per line. If that turns out to be too restrictive, the macro could be changed. That's probably unlikely, admittedly.
For some hardware these numbers could still be more than a 32-bit unsigned integer can hold, so the check is still needed.
Quoted 1 line
also an attempt from a hacker to try to crack the system.
This could be also v4l2-compliance setting the length field to -1. A warning is worth it only if there's good chance there's e.g. a kernel bug involved.
Signature · 4 lines
Kind regards, Sakari Ailus [email protected]
Re: [PATCH v2 1/3] videobuf2-core: Prevent size alignment wrapping buffer size to 0
Em Tue, 8 Jan 2019 15:38:32 +0200
Sakari Ailus <[email protected]> escreveu:
Quoted 77 lines
Hi Mauro,
Thanks for the review.
Quoted 59 lines · Mauro Carvalho Chehab
Em Tue, 8 Jan 2019 10:52:12 -0200
Mauro Carvalho Chehab <[email protected]> escreveu:Quoted 46 lines
Em Tue, 8 Jan 2019 10:58:34 +0200
Sakari Ailus <[email protected]> escreveu:Quoted 18 lines
PAGE_ALIGN()may wrap the buffer size around to 0. Prevent this by checking that the aligned value is not smaller than the unaligned one.Note on backporting to stable: the file used to be under
drivers/media/v4l2-core, it was moved to the current location after 4.14.Signed-off-by Sakari Ailus <[email protected]>Reviewed-by Hans Verkuil <[email protected]>--- drivers/media/common/videobuf2/videobuf2-core.c | 4 ++++ 1 file changed, 4 insertions(+)drivers/media/common/videobuf2/videobuf2-core.c +0
-0drivers/media/common/videobuf2/videobuf2-core.c static int __vb2_buf_mem_alloc(struct vb2_buffer *vb) · line 213
Quoted 7 lines
drivers/media/common/videobuf2/videobuf2-core.c +4
-0@@ -207,6 +207,10 @@ static int __vb2_buf_mem_alloc(struct vb2_buffer *vb)for (plane =0 ; plane < vb->num_planes; ++plane) {unsigned long size =PAGE_ALIGN (vb->planes[plane].length); +/* Did it wrap around? */ +if (size < vb->planes[plane].length) + goto free; +Sorry, but I can't see how this could ever happen (except for a very serious bug at the compiler or at the hardware).
See, the definition at
PAGE_ALIGNis (frommm.h):#define PAGE_ALIGN(addr) ALIGN(addr, PAGE_SIZE)and the macro it uses come from
kernel.h:#define __ALIGN_KERNEL(x, a) __ALIGN_KERNEL_MASK(x, (typeof(x))(a) - 1) #define __ALIGN_KERNEL_MASK(x, mask) (((x) + (mask)) & ~(mask)) .. #define ALIGN(x, a) __ALIGN_KERNEL((x), (a))So, this:
size = PAGE_ALIGN(length);Quoted 7 lines
(assuming PAGE_SIZE=
0x1000)becomes:
size = (length + 0x0fff) & ~0xfff;so, size will *always* be >= length.
Hmm... after looking at patch 2, now I understand what's your concern...
Quoted 8 lines
If someone indeed uses length =
INT_MAX, size will indeed be zero.Please adjust the description accordingly, as it doesn't reflect that.
Btw, in this particular case, I would use a
WARN_ON(), as this is something that indicates not only a driver bug (as the driver is letting someone to request a buffer a way too big), but probablyWhat's the maximum size a driver should allow? I guess this could be seen be a failure from the driver's part to limit the size of the buffer, but it's not trivial either to define that.
Quoted 8 lines
Hardware typically has maximum dimensions it can support, but the user may want to add padding at the end of the lines. Perhaps a helper macro could be used for this purpose: most likely there's no need to be more padding than there's image data per line. If that turns out to be too restrictive, the macro could be changed. That's probably unlikely, admittedly.
For some hardware these numbers could still be more than a 32-bit unsigned integer can hold, so the check is still needed.
I guess that, by changing from "int" to "unsigned long", we ensure that the number should be big enough to be able to represent the maximum allocation size.
On Linux, sizeof(long) is usually assumed to be sizeof(void *). Such assumption is used, for example, when we pass a structure pointer to ioctl's, instead of passing a long integer.
I mean, on a 64 bits system, long has 64 bits. AFAIKT, even the latest Xeon CPUs, the address space is lower than 64 bits. So, if one tries to allocate a memory with sizeof(ULONG_MAX), this will fail with ENOMEM.
On any (true) 32 bits system, the physical address is to 32 bits. So, if one tries to allocate a memory with ULONG_MAX, this should also fail, as there won't be memory for anything else.
There are some special cases, like X86_PAE (and ARM_LPAE). There, the physical address space is 64 bits, but instruction set is the 32 bits one. Yet, I'm almost sure that (at least on x86) a single memory block there can't be bigger than 32 bits.
What I'm trying to say is that I strongly suspect that we won't have any cases where someone using would need a buffer with more than 32 bits size on a non-64 architecture.
Quoted 7 lines
Quoted 2 lines
also an attempt from a hacker to try to crack the system.
This could be also v4l2-compliance setting the length field to -1. A warning is worth it only if there's good chance there's e.g. a kernel bug involved.
Thanks,
Mauro
Re: [PATCH v2 1/3] videobuf2-core: Prevent size alignment wrapping buffer size to 0
Quoted 105 lines · Mauro Carvalho Chehab
Em Tue, 8 Jan 2019 15:38:32 +0200
Sakari Ailus <[email protected]> escreveu:Quoted 77 lines
Hi Mauro,
Thanks for the review.
Quoted 59 lines · Mauro Carvalho Chehab
Em Tue, 8 Jan 2019 10:52:12 -0200
Mauro Carvalho Chehab <[email protected]> escreveu:Quoted 46 lines
Em Tue, 8 Jan 2019 10:58:34 +0200
Sakari Ailus <[email protected]> escreveu:Quoted 18 lines
PAGE_ALIGN()may wrap the buffer size around to 0. Prevent this by checking that the aligned value is not smaller than the unaligned one.Note on backporting to stable: the file used to be under
drivers/media/v4l2-core, it was moved to the current location after 4.14.Signed-off-by Sakari Ailus <[email protected]>Reviewed-by Hans Verkuil <[email protected]>--- drivers/media/common/videobuf2/videobuf2-core.c | 4 ++++ 1 file changed, 4 insertions(+)drivers/media/common/videobuf2/videobuf2-core.c +0
-0drivers/media/common/videobuf2/videobuf2-core.c static int __vb2_buf_mem_alloc(struct vb2_buffer *vb) · line 213
Quoted 7 lines
drivers/media/common/videobuf2/videobuf2-core.c +4
-0@@ -207,6 +207,10 @@ static int __vb2_buf_mem_alloc(struct vb2_buffer *vb)for (plane =0 ; plane < vb->num_planes; ++plane) {unsigned long size =PAGE_ALIGN (vb->planes[plane].length); +/* Did it wrap around? */ +if (size < vb->planes[plane].length) + goto free; +Sorry, but I can't see how this could ever happen (except for a very serious bug at the compiler or at the hardware).
See, the definition at
PAGE_ALIGNis (frommm.h):#define PAGE_ALIGN(addr) ALIGN(addr, PAGE_SIZE)and the macro it uses come from
kernel.h:#define __ALIGN_KERNEL(x, a) __ALIGN_KERNEL_MASK(x, (typeof(x))(a) - 1) #define __ALIGN_KERNEL_MASK(x, mask) (((x) + (mask)) & ~(mask)) .. #define ALIGN(x, a) __ALIGN_KERNEL((x), (a))So, this:
size = PAGE_ALIGN(length);Quoted 7 lines
(assuming PAGE_SIZE=
0x1000)becomes:
size = (length + 0x0fff) & ~0xfff;so, size will *always* be >= length.
Hmm... after looking at patch 2, now I understand what's your concern...
Quoted 8 lines
If someone indeed uses length =
INT_MAX, size will indeed be zero.Please adjust the description accordingly, as it doesn't reflect that.
Btw, in this particular case, I would use a
WARN_ON(), as this is something that indicates not only a driver bug (as the driver is letting someone to request a buffer a way too big), but probablyWhat's the maximum size a driver should allow? I guess this could be seen be a failure from the driver's part to limit the size of the buffer, but it's not trivial either to define that.
Quoted 8 lines
Hardware typically has maximum dimensions it can support, but the user may want to add padding at the end of the lines. Perhaps a helper macro could be used for this purpose: most likely there's no need to be more padding than there's image data per line. If that turns out to be too restrictive, the macro could be changed. That's probably unlikely, admittedly.
For some hardware these numbers could still be more than a 32-bit unsigned integer can hold, so the check is still needed.
I guess that, by changing from "int" to "unsigned long", we ensure that the number should be big enough to be able to represent the maximum allocation size.
On Linux, sizeof(long) is usually assumed to be
sizeof(void *). Such assumption is used, for example, when we pass a structure pointer to ioctl's, instead of passing a long integer.I mean, on a 64 bits system, long has 64 bits. AFAIKT, even the latest Xeon CPUs, the address space is lower than 64 bits. So, if one tries to allocate a memory with
sizeof(ULONG_MAX), this will fail with ENOMEM.On any (true) 32 bits system, the physical address is to 32 bits. So, if one tries to allocate a memory with
ULONG_MAX, this should also fail, as there won't be memory for anything else.
Quoted 8 lines
There are some special cases, like
X86_PAE(andARM_LPAE). There, the physical address space is 64 bits, but instruction set is the 32 bits one. Yet, I'm almost sure that (at least on x86) a single memory block there can't be bigger than 32 bits.What I'm trying to say is that I strongly suspect that we won't have any cases where someone using would need a buffer with more than 32 bits size on a non-64 architecture.
I agree; also the length field in struct v4l2_buffer is a __u32 so that limits the value range for size as well.
Signature · 2 lines
Sakari Ailus [email protected]
Re: [PATCH v2 1/3] videobuf2-core: Prevent size alignment wrapping buffer size to 0
In reply to Mauro Carvalho ChehabQuoted 55 lines · Mauro Carvalho Chehab
Em Tue, 8 Jan 2019 10:52:12 -0200
Mauro Carvalho Chehab <[email protected]> escreveu:Quoted 52 lines
Em Tue, 8 Jan 2019 10:58:34 +0200
Sakari Ailus <[email protected]> escreveu:Quoted 18 lines
PAGE_ALIGN()may wrap the buffer size around to 0. Prevent this by checking that the aligned value is not smaller than the unaligned one.Note on backporting to stable: the file used to be under
drivers/media/v4l2-core, it was moved to the current location after 4.14.Signed-off-by Sakari Ailus <[email protected]>Reviewed-by Hans Verkuil <[email protected]>--- drivers/media/common/videobuf2/videobuf2-core.c | 4 ++++ 1 file changed, 4 insertions(+)drivers/media/common/videobuf2/videobuf2-core.c +0
-0drivers/media/common/videobuf2/videobuf2-core.c static int __vb2_buf_mem_alloc(struct vb2_buffer *vb) · line 213
Quoted 7 lines
drivers/media/common/videobuf2/videobuf2-core.c +4
-0@@ -207,6 +207,10 @@ static int __vb2_buf_mem_alloc(struct vb2_buffer *vb)for (plane =0 ; plane < vb->num_planes; ++plane) {unsigned long size =PAGE_ALIGN (vb->planes[plane].length); +/* Did it wrap around? */ +if (size < vb->planes[plane].length) + goto free; +Sorry, but I can't see how this could ever happen (except for a very serious bug at the compiler or at the hardware).
See, the definition at
PAGE_ALIGNis (frommm.h):#define PAGE_ALIGN(addr) ALIGN(addr, PAGE_SIZE)and the macro it uses come from
kernel.h:#define __ALIGN_KERNEL(x, a) __ALIGN_KERNEL_MASK(x, (typeof(x))(a) - 1) #define __ALIGN_KERNEL_MASK(x, mask) (((x) + (mask)) & ~(mask)) .. #define ALIGN(x, a) __ALIGN_KERNEL((x), (a))So, this:
size = PAGE_ALIGN(length);(assuming PAGE_SIZE=
0x1000)becomes:
size = (length + 0x0fff) & ~0xfff;
Quoted 8 lines
Quoted 1 line
so, size will *always* be >= length.
Hmm... after looking at patch 2, now I understand what's your concern...
If someone indeed uses length =
INT_MAX, size will indeed be zero.Please adjust the description accordingly, as it doesn't reflect that.
How about:
PAGE_ALIGN() may wrap the buffer length around to 0 if the value to be aligned is close to the top of the value range of the type. Prevent this by checking that the aligned value is not smaller than the unaligned one.
Signature · 2 lines
Sakari Ailus [email protected]
Re: [PATCH v2 1/3] videobuf2-core: Prevent size alignment wrapping buffer size to 0
Em Tue, 8 Jan 2019 15:40:47 +0200
Sakari Ailus <[email protected]> escreveu:
Quoted 62 lines · Mauro Carvalho Chehab
Quoted 61 lines
Em Tue, 8 Jan 2019 10:52:12 -0200
Mauro Carvalho Chehab <[email protected]> escreveu:Quoted 46 lines
Em Tue, 8 Jan 2019 10:58:34 +0200
Sakari Ailus <[email protected]> escreveu:Quoted 18 lines
PAGE_ALIGN()may wrap the buffer size around to 0. Prevent this by checking that the aligned value is not smaller than the unaligned one.Note on backporting to stable: the file used to be under
drivers/media/v4l2-core, it was moved to the current location after 4.14.Signed-off-by Sakari Ailus <[email protected]>Reviewed-by Hans Verkuil <[email protected]>--- drivers/media/common/videobuf2/videobuf2-core.c | 4 ++++ 1 file changed, 4 insertions(+)drivers/media/common/videobuf2/videobuf2-core.c +0
-0drivers/media/common/videobuf2/videobuf2-core.c static int __vb2_buf_mem_alloc(struct vb2_buffer *vb) · line 213
Quoted 7 lines
drivers/media/common/videobuf2/videobuf2-core.c +4
-0@@ -207,6 +207,10 @@ static int __vb2_buf_mem_alloc(struct vb2_buffer *vb)for (plane =0 ; plane < vb->num_planes; ++plane) {unsigned long size =PAGE_ALIGN (vb->planes[plane].length); +/* Did it wrap around? */ +if (size < vb->planes[plane].length) + goto free; +Sorry, but I can't see how this could ever happen (except for a very serious bug at the compiler or at the hardware).
See, the definition at
PAGE_ALIGNis (frommm.h):#define PAGE_ALIGN(addr) ALIGN(addr, PAGE_SIZE)and the macro it uses come from
kernel.h:#define __ALIGN_KERNEL(x, a) __ALIGN_KERNEL_MASK(x, (typeof(x))(a) - 1) #define __ALIGN_KERNEL_MASK(x, mask) (((x) + (mask)) & ~(mask)) .. #define ALIGN(x, a) __ALIGN_KERNEL((x), (a))So, this:
size = PAGE_ALIGN(length);Quoted 7 lines
(assuming PAGE_SIZE=
0x1000)becomes:
size = (length + 0x0fff) & ~0xfff;so, size will *always* be >= length.
Hmm... after looking at patch 2, now I understand what's your concern...
If someone indeed uses length =
INT_MAX, size will indeed be zero.
Quoted 8 lines
Quoted 2 lines
Please adjust the description accordingly, as it doesn't reflect that.
How about:
PAGE_ALIGN()may wrap the buffer length around to 0 if the value to be aligned is close to the top of the value range of the type. Prevent this by checking that the aligned value is not smaller than the unaligned one.
I would be a way more clear, as this is there to prevent a single special case: length == ULEN_MAX. Something like:
If one tried to allocate a buffer with sizeof(ULEN_MAX), this will cause an overflow at PAGE_ALIGN(), making it return zero as the size of the buffer, causing the code to fail.
I would even let it clearer at the code itself. So, instead of the hunk you proposed, I would do:
unsigned long size = vb->planes[plane].length; /* Prevent PAGE_ALIGN overflow */ if (WARN_ON(size == ULONG_MAX)) goto free; size = PAGE_ALIGN(vb->planes[plane].length);
Thanks,
Mauro
Re: [PATCH v2 1/3] videobuf2-core: Prevent size alignment wrapping buffer size to 0
Quoted 51 lines · On Tuesday, 8 January 2019 16:30:22 EET Mauro Carvalho Chehab wrote:
Em Tue, 8 Jan 2019 15:40:47 +0200
Sakari Ailus <[email protected]> escreveu:
Quoted 48 lines · Mauro Carvalho Chehab
Quoted 47 lines
Em Tue, 8 Jan 2019 10:52:12 -0200
Mauro Carvalho Chehab <[email protected]> escreveu:
Quoted 44 lines
Em Tue, 8 Jan 2019 10:58:34 +0200
Sakari Ailus <[email protected]> escreveu:
Quoted 22 lines
PAGE_ALIGN()may wrap the buffer size around to 0. Prevent this by checking that the aligned value is not smaller than the unaligned one.Note on backporting to stable: the file used to be under
drivers/media/v4l2-core, it was moved to the current location after 4.14.Signed-off-by Sakari Ailus <[email protected]>Reviewed-by Hans Verkuil <[email protected]>---
drivers/media/common/videobuf2/videobuf2-core.c | 4 ++++ 1 file changed, 4 insertions(+)(quoted diff) +0
-0
b/drivers/media/common/videobuf2/videobuf2-core.cindex0ca81d495bda..0234ddbfa4de100644drivers/media/common/videobuf2/videobuf2-core.c static int __vb2_buf_mem_alloc(struct · line 216
Quoted 10 lines
drivers/media/common/videobuf2/videobuf2-core.c +4
-0@@ -207,6 +207,10 @@ static int __vb2_buf_mem_alloc(struct vb2_buffer *vb)for (plane =0 ; plane < vb->num_planes; ++plane) {unsigned long size =PAGE_ALIGN (vb->planes[plane].length); +/* Did it wrap around? */ +if (size < vb->planes[plane].length) + goto free; +Sorry, but I can't see how this could ever happen (except for a very serious bug at the compiler or at the hardware).
See, the definition at
PAGE_ALIGNis (frommm.h):#define PAGE_ALIGN(addr) ALIGN(addr, PAGE_SIZE)and the macro it uses come from
kernel.h:#define __ALIGN_KERNEL(x, a) __ALIGN_KERNEL_MASK(x, (typeof(x))
(a) -
Quoted 36 lines
Quoted 22 lines
Quoted 13 lines
Quoted 13 lines
1) #define __ALIGN_KERNEL_MASK(x, mask) (((x) + (mask)) & ~(mask)) .. #define ALIGN(x, a) __ALIGN_KERNEL((x), (a))So, this:
size = PAGE_ALIGN(length);(assuming PAGE_SIZE=
0x1000)becomes:
size = (length + 0x0fff) & ~0xfff;Quoted 8 lines
Quoted 1 line
so, size will *always* be >= length.
Hmm... after looking at patch 2, now I understand what's your concern...
If someone indeed uses length =
INT_MAX, size will indeed be zero.Please adjust the description accordingly, as it doesn't reflect that.
Quoted 6 lines
How about:
PAGE_ALIGN()may wrap the buffer length around to 0 if the value to be aligned is close to the top of the value range of the type. Prevent this by
checking that the aligned value is not smaller than the unaligned one.I would be a way more clear, as this is there to prevent a single special case: length ==
ULEN_MAX. Something like:If one tried to allocate a buffer with
sizeof(ULEN_MAX), this will cause an overflow atPAGE_ALIGN(), making it return zero as the size of the buffer, causing the code to fail.
Quoted 8 lines
I would even let it clearer at the code itself. So, instead of the hunk you proposed, I would do:
unsigned long size = vb->planes[plane].length; /* Prevent PAGE_ALIGN overflow */ if (WARN_ON(size == ULONG_MAX)) goto free;
ULONG_MAX - PAGE_SIZE + 2 to ULONG_MAX would all cause the same issue.
Quoted 2 lines
size = PAGE_ALIGN(vb->planes[plane].length);
Signature · 3 lines
Regards, Laurent Pinchart
Re: [PATCH v2 1/3] videobuf2-core: Prevent size alignment wrapping buffer size to 0
Em Tue, 08 Jan 2019 18:05:57 +0200
Laurent Pinchart <[email protected]> escreveu:
Quoted 92 lines · On Tuesday, 8 January 2019 16:30:22 EET Mauro Carvalho Chehab wrote:
Quoted 51 lines
Em Tue, 8 Jan 2019 15:40:47 +0200
Sakari Ailus <[email protected]> escreveu:
Quoted 48 lines · Mauro Carvalho Chehab
Quoted 47 lines
Em Tue, 8 Jan 2019 10:52:12 -0200
Mauro Carvalho Chehab <[email protected]> escreveu:
Quoted 44 lines
Em Tue, 8 Jan 2019 10:58:34 +0200
Sakari Ailus <[email protected]> escreveu:
Quoted 22 lines
PAGE_ALIGN()may wrap the buffer size around to 0. Prevent this by checking that the aligned value is not smaller than the unaligned one.Note on backporting to stable: the file used to be under
drivers/media/v4l2-core, it was moved to the current location after 4.14.Signed-off-by Sakari Ailus <[email protected]>Reviewed-by Hans Verkuil <[email protected]>---
drivers/media/common/videobuf2/videobuf2-core.c | 4 ++++ 1 file changed, 4 insertions(+)(quoted diff) +0
-0
b/drivers/media/common/videobuf2/videobuf2-core.cindex0ca81d495bda..0234ddbfa4de100644drivers/media/common/videobuf2/videobuf2-core.c static int __vb2_buf_mem_alloc(struct · line 216
Quoted 10 lines
drivers/media/common/videobuf2/videobuf2-core.c +4
-0@@ -207,6 +207,10 @@ static int __vb2_buf_mem_alloc(struct vb2_buffer *vb)for (plane =0 ; plane < vb->num_planes; ++plane) {unsigned long size =PAGE_ALIGN (vb->planes[plane].length); +/* Did it wrap around? */ +if (size < vb->planes[plane].length) + goto free; +Sorry, but I can't see how this could ever happen (except for a very serious bug at the compiler or at the hardware).
See, the definition at
PAGE_ALIGNis (frommm.h):#define PAGE_ALIGN(addr) ALIGN(addr, PAGE_SIZE)and the macro it uses come from
kernel.h:#define __ALIGN_KERNEL(x, a) __ALIGN_KERNEL_MASK(x, (typeof(x))(a) -
Quoted 39 lines
Quoted 22 lines
Quoted 13 lines
Quoted 13 lines
1) #define __ALIGN_KERNEL_MASK(x, mask) (((x) + (mask)) & ~(mask)) .. #define ALIGN(x, a) __ALIGN_KERNEL((x), (a))So, this:
size = PAGE_ALIGN(length);(assuming PAGE_SIZE=
0x1000)becomes:
size = (length + 0x0fff) & ~0xfff;Quoted 8 lines
Quoted 1 line
so, size will *always* be >= length.
Hmm... after looking at patch 2, now I understand what's your concern...
If someone indeed uses length =
INT_MAX, size will indeed be zero.Please adjust the description accordingly, as it doesn't reflect that.
Quoted 6 lines
How about:
PAGE_ALIGN()may wrap the buffer length around to 0 if the value to be aligned is close to the top of the value range of the type. Prevent this by
checking that the aligned value is not smaller than the unaligned one.I would be a way more clear, as this is there to prevent a single special case: length ==
ULEN_MAX. Something like:If one tried to allocate a buffer with
sizeof(ULEN_MAX), this will cause an overflow atPAGE_ALIGN(), making it return zero as the size of the buffer, causing the code to fail.I would even let it clearer at the code itself. So, instead of the hunk you proposed, I would do:
Quoted 7 lines
Quoted 5 lines
unsigned long size = vb->planes[plane].length; /* Prevent PAGE_ALIGN overflow */ if (WARN_ON(size == ULONG_MAX)) goto free;
ULONG_MAX-PAGE_SIZE+ 2 toULONG_MAXwould all cause the same issue.
True. The actual check should be:
if (WARN_ON(size > ULONG_MAX - PAGE_SIZE + 1))
Not that the original proposal of checking after the overflow is wrong, but, IMHO, something linking the size to ULONG_MAX makes clearer about what kind of issue the code is meant to solve. A good comment before that would work fine.
Thanks,
Mauro
Re: [PATCH v2 1/3] videobuf2-core: Prevent size alignment wrapping buffer size to 0
Hi Mauro,
Quoted 101 lines · Mauro Carvalho Chehab
Em Tue, 08 Jan 2019 18:05:57 +0200
Laurent Pinchart <[email protected]> escreveu:Quoted 98 lines · On Tuesday, 8 January 2019 16:30:22 EET Mauro Carvalho Chehab wrote:
Quoted 51 lines
Em Tue, 8 Jan 2019 15:40:47 +0200
Sakari Ailus <[email protected]> escreveu:
Quoted 48 lines · Mauro Carvalho Chehab
Quoted 47 lines
Em Tue, 8 Jan 2019 10:52:12 -0200
Mauro Carvalho Chehab <[email protected]> escreveu:
Quoted 44 lines
Em Tue, 8 Jan 2019 10:58:34 +0200
Sakari Ailus <[email protected]> escreveu:
Quoted 22 lines
PAGE_ALIGN()may wrap the buffer size around to 0. Prevent this by checking that the aligned value is not smaller than the unaligned one.Note on backporting to stable: the file used to be under
drivers/media/v4l2-core, it was moved to the current location after 4.14.Signed-off-by Sakari Ailus <[email protected]>Reviewed-by Hans Verkuil <[email protected]>---
drivers/media/common/videobuf2/videobuf2-core.c | 4 ++++ 1 file changed, 4 insertions(+)(quoted diff) +0
-0
b/drivers/media/common/videobuf2/videobuf2-core.cindex0ca81d495bda..0234ddbfa4de100644drivers/media/common/videobuf2/videobuf2-core.c static int __vb2_buf_mem_alloc(struct · line 216
Quoted 10 lines
drivers/media/common/videobuf2/videobuf2-core.c +4
-0@@ -207,6 +207,10 @@ static int __vb2_buf_mem_alloc(struct vb2_buffer *vb)for (plane =0 ; plane < vb->num_planes; ++plane) {unsigned long size =PAGE_ALIGN (vb->planes[plane].length); +/* Did it wrap around? */ +if (size < vb->planes[plane].length) + goto free; +Sorry, but I can't see how this could ever happen (except for a very serious bug at the compiler or at the hardware).
See, the definition at
PAGE_ALIGNis (frommm.h):#define PAGE_ALIGN(addr) ALIGN(addr, PAGE_SIZE)and the macro it uses come from
kernel.h:#define __ALIGN_KERNEL(x, a) __ALIGN_KERNEL_MASK(x, (typeof(x))(a) -
Quoted 44 lines
Quoted 22 lines
Quoted 13 lines
Quoted 13 lines
1) #define __ALIGN_KERNEL_MASK(x, mask) (((x) + (mask)) & ~(mask)) .. #define ALIGN(x, a) __ALIGN_KERNEL((x), (a))So, this:
size = PAGE_ALIGN(length);(assuming PAGE_SIZE=
0x1000)becomes:
size = (length + 0x0fff) & ~0xfff;Quoted 8 lines
Quoted 1 line
so, size will *always* be >= length.
Hmm... after looking at patch 2, now I understand what's your concern...
If someone indeed uses length =
INT_MAX, size will indeed be zero.Please adjust the description accordingly, as it doesn't reflect that.
Quoted 6 lines
How about:
PAGE_ALIGN()may wrap the buffer length around to 0 if the value to be aligned is close to the top of the value range of the type. Prevent this by
checking that the aligned value is not smaller than the unaligned one.I would be a way more clear, as this is there to prevent a single special case: length ==
ULEN_MAX. Something like:If one tried to allocate a buffer with
sizeof(ULEN_MAX), this will cause an overflow atPAGE_ALIGN(), making it return zero as the size of the buffer, causing the code to fail.I would even let it clearer at the code itself. So, instead of the hunk you proposed, I would do:
unsigned long size = vb->planes[plane].length; /* Prevent PAGE_ALIGN overflow */ if (WARN_ON(size == ULONG_MAX)) goto free;
Quoted 5 lines
Quoted 1 line
ULONG_MAX-PAGE_SIZE+ 2 toULONG_MAXwould all cause the same issue.True. The actual check should be:
if (
WARN_ON(size > ULONG_MAX - PAGE_SIZE + 1))
That is also wrong: aligning ULONG_MAX - PAGE_SIZE + 1 to page is 0, too.
Quoted 5 lines
Not that the original proposal of checking after the overflow is wrong, but, IMHO, something linking the size to
ULONG_MAXmakes clearer about what kind of issue the code is meant to solve. A good comment before that would work fine.
I find the original test more simple to grasp: it is independent of how the alignment is done, to PAGE_SIZE or to something else. It was also right from the start unlike the two other checks that have been proposed so far. Therefore my preference to stick to that. The slight downside is that it is not apparent from the problem is related to aligning to page, but that is mitigated by the comment added by this patch.
This property of PAGE_ALIGN() is not usually relevant as typically it's not used to align addresses on the last page (of the value range).
Signature · 4 lines
Kind regards, Sakari Ailus [email protected]
PATCH v2 2/3 videobuf2-dma-sg: Prevent size from overflowing
In reply to Sakari Ailusbuf->size is an unsigned long; casting that to int will lead to an overflow if buf->size exceeds INT_MAX.
Fix this by changing the type to unsigned long instead. This is possible as the buf->size is always aligned to PAGE_SIZE, and therefore the size will never have values lesser than 0.
Note on backporting to stable: the file used to be under drivers/media/v4l2-core, it was moved to the current location after 4.14.
--- drivers/media/common/videobuf2/videobuf2-dma-sg.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)
drivers/media/common/videobuf2/videobuf2-dma-sg.c +4 -1
@@ -59,7 +59,10 @@ static int vb2_dma_sg_alloc_compacted(struct vb2_dma_sg_buf *buf, gfp_t gfp_flags) {unsigned int last_page =0 ; -int size = buf->size; +unsigned long size = buf->size; + +if (WARN_ON (size & ~PAGE_MASK )) +return -ENOMEM ;while (size >0 ) {struct page *pages;
Signature · 1 line
2.11.0
Re: [PATCH v2 2/3] videobuf2-dma-sg: Prevent size from overflowing
Em Tue, 8 Jan 2019 10:58:35 +0200
Sakari Ailus <[email protected]> escreveu:
Quoted 22 lines
buf->sizeis an unsigned long; casting that to int will lead to an overflow ifbuf->sizeexceedsINT_MAX.Fix this by changing the type to unsigned long instead. This is possible as the
buf->sizeis always aligned toPAGE_SIZE, and therefore the size will never have values lesser than 0.Note on backporting to stable: the file used to be under
drivers/media/v4l2-core, it was moved to the current location after 4.14.Signed-off-by Sakari Ailus <[email protected]>Reviewed-by Hans Verkuil <[email protected]>--- drivers/media/common/videobuf2/videobuf2-dma-sg.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)drivers/media/common/videobuf2/videobuf2-dma-sg.c +0
-0
drivers/media/common/videobuf2/videobuf2-dma-sg.c static int vb2_dma_sg_alloc_compacted(struct vb2_dma_sg_buf *buf, · line 62
Quoted 5 lines
drivers/media/common/videobuf2/videobuf2-dma-sg.c +1
-1@@ -59,7 +59,10 @@ static int vb2_dma_sg_alloc_compacted(struct vb2_dma_sg_buf *buf, gfp_t gfp_flags) {unsigned int last_page =0 ; -int size = buf->size; +unsigned long size = buf->size;
OK.
drivers/media/common/videobuf2/videobuf2-dma-sg.c static int vb2_dma_sg_alloc_compacted(struct vb2_dma_sg_buf *buf, · line 65
Quoted 3 lines
drivers/media/common/videobuf2/videobuf2-dma-sg.c +3
-0+ +if (WARN_ON (size & ~PAGE_MASK )) +return -ENOMEM ;
Hmm... why do we need a warn on here? This is called by this code:
static int __vb2_buf_mem_alloc(struct vb2_buffer *vb)
{
struct vb2_queue *q = vb->vb2_queue;
void *mem_priv;
int plane;
int ret = -ENOMEM;
/*
* Allocate memory for all planes in this buffer
* NOTE: mmapped areas should be page aligned
*/
for (plane = 0; plane < vb->num_planes; ++plane) {
unsigned long size = PAGE_ALIGN(vb->planes[plane].length);
mem_priv = call_ptr_memop(vb, alloc,
q->alloc_devs[plane] ? : q->dev,
q->dma_attrs, size, q->dma_dir, q->gfp_flags);With already insures that size is page aligned.
Thanks,
Mauro
Re: [PATCH v2 2/3] videobuf2-dma-sg: Prevent size from overflowing
Quoted 33 lines · Mauro Carvalho Chehab
Em Tue, 8 Jan 2019 10:58:35 +0200
Sakari Ailus <[email protected]> escreveu:Quoted 22 lines
buf->sizeis an unsigned long; casting that to int will lead to an overflow ifbuf->sizeexceedsINT_MAX.Fix this by changing the type to unsigned long instead. This is possible as the
buf->sizeis always aligned toPAGE_SIZE, and therefore the size will never have values lesser than 0.Note on backporting to stable: the file used to be under
drivers/media/v4l2-core, it was moved to the current location after 4.14.Signed-off-by Sakari Ailus <[email protected]>Reviewed-by Hans Verkuil <[email protected]>--- drivers/media/common/videobuf2/videobuf2-dma-sg.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)drivers/media/common/videobuf2/videobuf2-dma-sg.c +0
-0drivers/media/common/videobuf2/videobuf2-dma-sg.c static int vb2_dma_sg_alloc_compacted(struct vb2_dma_sg_buf *buf, · line 62
Quoted 5 lines
drivers/media/common/videobuf2/videobuf2-dma-sg.c +1
-1@@ -59,7 +59,10 @@ static int vb2_dma_sg_alloc_compacted(struct vb2_dma_sg_buf *buf, gfp_t gfp_flags) {unsigned int last_page =0 ; -int size = buf->size; +unsigned long size = buf->size;OK.
Quoted 5 lines
drivers/media/common/videobuf2/videobuf2-dma-sg.c static int vb2_dma_sg_alloc_compacted(struct vb2_dma_sg_buf *buf, · line 65
Quoted 3 lines
drivers/media/common/videobuf2/videobuf2-dma-sg.c +3
-0+ +if (WARN_ON (size & ~PAGE_MASK )) +return -ENOMEM ;Hmm... why do we need a warn on here? This is called by this code:
This was suggested as a sanity check in review of v1 of the set.
Supposing that someone once removed that alignment, things would go rather completely haywire. There would probably be lots of other troubles as well but this one would probably corrupt system memory (at least).
Quoted 23 lines
static int
__vb2_buf_mem_alloc(struct vb2_buffer *vb){ struct vb2_queue *q = vb->vb2_queue; void *mem_priv; int plane; int ret = -ENOMEM; /* * Allocate memory for all planes in this buffer * NOTE: mmapped areas should be page aligned */ for (plane = 0; plane < vb->num_planes; ++plane) { unsigned long size = PAGE_ALIGN(vb->planes[plane].length); mem_priv = call_ptr_memop(vb, alloc, q->alloc_devs[plane] ? : q->dev, q->dma_attrs, size, q->dma_dir, q->gfp_flags);With already insures that size is page aligned.
Thanks,
Mauro
Signature · 2 lines
Sakari Ailus [email protected]
Re: [PATCH v2 2/3] videobuf2-dma-sg: Prevent size from overflowing
Em Tue, 8 Jan 2019 15:29:26 +0200
Sakari Ailus <[email protected]> escreveu:
Quoted 40 lines · Mauro Carvalho Chehab
Quoted 33 lines
Em Tue, 8 Jan 2019 10:58:35 +0200
Sakari Ailus <[email protected]> escreveu:Quoted 22 lines
buf->sizeis an unsigned long; casting that to int will lead to an overflow ifbuf->sizeexceedsINT_MAX.Fix this by changing the type to unsigned long instead. This is possible as the
buf->sizeis always aligned toPAGE_SIZE, and therefore the size will never have values lesser than 0.Note on backporting to stable: the file used to be under
drivers/media/v4l2-core, it was moved to the current location after 4.14.Signed-off-by Sakari Ailus <[email protected]>Reviewed-by Hans Verkuil <[email protected]>--- drivers/media/common/videobuf2/videobuf2-dma-sg.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)drivers/media/common/videobuf2/videobuf2-dma-sg.c +0
-0drivers/media/common/videobuf2/videobuf2-dma-sg.c static int vb2_dma_sg_alloc_compacted(struct vb2_dma_sg_buf *buf, · line 62
Quoted 5 lines
drivers/media/common/videobuf2/videobuf2-dma-sg.c +1
-1@@ -59,7 +59,10 @@ static int vb2_dma_sg_alloc_compacted(struct vb2_dma_sg_buf *buf, gfp_t gfp_flags) {unsigned int last_page =0 ; -int size = buf->size; +unsigned long size = buf->size;OK.
Quoted 5 lines
drivers/media/common/videobuf2/videobuf2-dma-sg.c static int vb2_dma_sg_alloc_compacted(struct vb2_dma_sg_buf *buf, · line 65
Quoted 3 lines
drivers/media/common/videobuf2/videobuf2-dma-sg.c +3
-0+ +if (WARN_ON (size & ~PAGE_MASK )) +return -ENOMEM ;Hmm... why do we need a warn on here? This is called by this code:
Quoted 5 lines
This was suggested as a sanity check in review of v1 of the set.
Supposing that someone once removed that alignment, things would go rather completely haywire. There would probably be lots of other troubles as well but this one would probably corrupt system memory (at least).
Well, patch 3 prevents that. See: this is not like something that driver developers can mess with that, as the only place where the .alloc() ops is called is by the VB2 core, and it already ensures page alignment.
If one would ever try to remove PAGE_ALIGN() from vb2 core, we'll nack it, as we know that such change will break things.
Thanks,
Mauro
Re: [PATCH v2 2/3] videobuf2-dma-sg: Prevent size from overflowing
Quoted 49 lines · Mauro Carvalho Chehab
Em Tue, 8 Jan 2019 15:29:26 +0200
Sakari Ailus <[email protected]> escreveu:Quoted 40 lines · Mauro Carvalho Chehab
Quoted 33 lines
Em Tue, 8 Jan 2019 10:58:35 +0200
Sakari Ailus <[email protected]> escreveu:Quoted 22 lines
buf->sizeis an unsigned long; casting that to int will lead to an overflow ifbuf->sizeexceedsINT_MAX.Fix this by changing the type to unsigned long instead. This is possible as the
buf->sizeis always aligned toPAGE_SIZE, and therefore the size will never have values lesser than 0.Note on backporting to stable: the file used to be under
drivers/media/v4l2-core, it was moved to the current location after 4.14.Signed-off-by Sakari Ailus <[email protected]>Reviewed-by Hans Verkuil <[email protected]>--- drivers/media/common/videobuf2/videobuf2-dma-sg.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)drivers/media/common/videobuf2/videobuf2-dma-sg.c +0
-0drivers/media/common/videobuf2/videobuf2-dma-sg.c static int vb2_dma_sg_alloc_compacted(struct vb2_dma_sg_buf *buf, · line 62
Quoted 5 lines
drivers/media/common/videobuf2/videobuf2-dma-sg.c +1
-1@@ -59,7 +59,10 @@ static int vb2_dma_sg_alloc_compacted(struct vb2_dma_sg_buf *buf, gfp_t gfp_flags) {unsigned int last_page =0 ; -int size = buf->size; +unsigned long size = buf->size;OK.
Quoted 5 lines
drivers/media/common/videobuf2/videobuf2-dma-sg.c static int vb2_dma_sg_alloc_compacted(struct vb2_dma_sg_buf *buf, · line 65
Quoted 3 lines
drivers/media/common/videobuf2/videobuf2-dma-sg.c +3
-0+ +if (WARN_ON (size & ~PAGE_MASK )) +return -ENOMEM ;Hmm... why do we need a warn on here? This is called by this code:
Quoted 5 lines
This was suggested as a sanity check in review of v1 of the set.
Supposing that someone once removed that alignment, things would go rather completely haywire. There would probably be lots of other troubles as well but this one would probably corrupt system memory (at least).
Quoted 6 lines
Well, patch 3 prevents that. See: this is not like something that driver developers can mess with that, as the only place where the .alloc() ops is called is by the VB2 core, and it already ensures page alignment.
If one would ever try to remove
PAGE_ALIGN()from vb2 core, we'll nack it, as we know that such change will break things.
Indeed. I'm certainly fine with dropping the sanity check; I think there are enough warnings elsewhere plus common sense to avoid making such a change.
Signature · 2 lines
Sakari Ailus [email protected]
PATCH v2 3/3 videobuf2-core.h: Document the alloc memop size argument as page aligned
In reply to Sakari AilusThe size argument of the alloc memop, which allocates buffer memory, is page aligned. Document it as such in the only caller as well as ops documentation.
--- drivers/media/common/videobuf2/videobuf2-core.c | 1 + include/media/videobuf2-core.h | 3 ++- 2 files changed, 3 insertions(+), 1 deletion(-)
drivers/media/common/videobuf2/videobuf2-core.c +1 -0
@@ -205,6 +205,7 @@ static int __vb2_buf_mem_alloc(struct vb2_buffer *vb)* NOTE: mmapped areas should be page aligned */ for (plane =0 ; plane < vb->num_planes; ++plane) { +/* Memops alloc requires size to be page aligned. */ unsigned long size =PAGE_ALIGN (vb->planes[plane].length);/* Did it wrap around? */
include/media/videobuf2-core.h +2 -1
@@ -54,7 +54,8 @@ struct vb2_threadio_data;* will then be passed as @buf_priv argument to other ops in this * structure. Additional gfp_flags to use when allocating the * are also passed to this operation. These flags are from the -* gfp_flags field of vb2_queue. +* gfp_flags field of vb2_queue. The size argument to this function +* shall be *page aligned*. * @put: inform the allocator that the buffer will no longer be used; * usually will result in the allocator freeing the buffer (if * no other users of this buffer are present); the @buf_priv
Signature · 1 line
2.11.0