[PATCH] cxgb4: fix undefined behavior in mem.c
In function `c4iw_dealloc_mw`, variable mhp's value is printed after
freed, which triggers undefined behavior according to this post:
https://trust-in-soft.com/dangling-pointer-indeterminate/.
This commit fixes it by swapping the order of `kfree` and `pr_debug`.
Signed-off-by: Shaobo He
---
drivers/infiniband/hw/cxgb4/mem.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/infiniband/hw/cxgb4/mem.c
b/drivers/infiniband/hw/cxgb4/mem.c
index 7b76e6f..bb8e0bc 100644
--- a/drivers/infiniband/hw/cxgb4/mem.c
+++ b/drivers/infiniband/hw/cxgb4/mem.c
@@ -684,8 +684,8 @@ int c4iw_dealloc_mw(struct ib_mw *mw)
mhp->wr_waitp);
kfree_skb(mhp->dereg_skb);
c4iw_put_wr_wait(mhp->wr_waitp);
- kfree(mhp);
pr_debug("ib_mw %p mmid 0x%x ptr %p\n", mw, mmid, mhp);
+ kfree(mhp);
return 0;
}
--
2.7.4
[PATCH] platform/sh_veu.c: remove redundant NULL pointer checks
Function `sh_veu_find_fmt` returns an address that is an addition of a base pointer `sh_veu_fmt` and an offset. The base pointer refers to a global variable of which address cannot be NULL. Therefore, this commit removes the NULL pointer checks on the return values of function `sh_veu_find_fmt`. Signed-off-by: Shaobo He --- drivers/media/platform/sh_veu.c | 6 -- 1 file changed, 6 deletions(-) diff --git a/drivers/media/platform/sh_veu.c b/drivers/media/platform/sh_veu.c index 09ae64a..3069015 100644 --- a/drivers/media/platform/sh_veu.c +++ b/drivers/media/platform/sh_veu.c @@ -493,9 +493,6 @@ static int sh_veu_try_fmt_vid_cap(struct file *file, void *priv, const struct sh_veu_format *fmt; fmt = sh_veu_find_fmt(f); - if (!fmt) - /* wrong buffer type */ - return -EINVAL; return sh_veu_try_fmt(f, fmt); } @@ -506,9 +503,6 @@ static int sh_veu_try_fmt_vid_out(struct file *file, void *priv, const struct sh_veu_format *fmt; fmt = sh_veu_find_fmt(f); - if (!fmt) - /* wrong buffer type */ - return -EINVAL; return sh_veu_try_fmt(f, fmt); } -- 2.7.4
Re: [PATCH] cxgb4: fix undefined behavior in mem.c
I can't afford a pdf version of the C standard. So I looked at the draft version
used in the link I put in the commit message. It says (in 6.2.4:2),
```
The lifetime of an object is the portion of program execution during which
storage is guaranteed to be reserved for it. An object exists, has a constant
address, and retains its last-stored value throughout its lifetime. If an object
is referred to outside of its lifetime, the behavior is undefined. The value of
a pointer becomes indeterminate when the object it points to (or just past)
reaches the end of its lifetime.
```
I couldn't find the definition of lifetime over a dynamically allocated object
in the draft of C standard. I refer to this link
(https://en.cppreference.com/w/c/language/lifetime) which suggests that the
lifetime of an allocated object ends after the deallocation function is called
upon it.
I think maybe the more problematic issue is that the value of a freed pointer is
intermediate.
Shaobo
On 2/28/19 3:56 PM, Bart Van Assche wrote:
On Thu, 2019-02-28 at 15:38 -0700, Shaobo He wrote:
In function `c4iw_dealloc_mw`, variable mhp's value is printed after
freed, which triggers undefined behavior according to this post:
https://trust-in-soft.com/dangling-pointer-indeterminate/.
This commit fixes it by swapping the order of `kfree` and `pr_debug`.
Signed-off-by: Shaobo He
---
drivers/infiniband/hw/cxgb4/mem.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/infiniband/hw/cxgb4/mem.c
b/drivers/infiniband/hw/cxgb4/mem.c
index 7b76e6f..bb8e0bc 100644
--- a/drivers/infiniband/hw/cxgb4/mem.c
+++ b/drivers/infiniband/hw/cxgb4/mem.c
@@ -684,8 +684,8 @@ int c4iw_dealloc_mw(struct ib_mw *mw)
mhp->wr_waitp);
kfree_skb(mhp->dereg_skb);
c4iw_put_wr_wait(mhp->wr_waitp);
- kfree(mhp);
pr_debug("ib_mw %p mmid 0x%x ptr %p\n", mw, mmid, mhp);
+ kfree(mhp);
return 0;
}
Please quote the relevant paragraphs from the C standard. All I have found
about free() in ISO/IEC 9899:2017 is the following:
Description
The free function causes the space pointed to by ptr to be deallocated, that
is, made available for further allocation. If ptr is a null pointer, no
action occurs. Otherwise, if the argument does not match a pointer earlier
returned by a memory management function, or if the space has been
deallocated by a call to free or realloc, the behavior is undefined.
That is not sufficient to claim that the above code triggers undefined
behavior.
Bart.
Re: [PATCH] cxgb4: fix undefined behavior in mem.c
Good catch. But if we agree on that memory management functions are those specified by the C standard, would it be OK to ignore so-called use after free or double free bugs for the kernel as C standard does not apply to kfree? On 2/28/19 4:33 PM, Bart Van Assche wrote: On Thu, 2019-02-28 at 16:18 -0700, Shaobo He wrote: I can't afford a pdf version of the C standard. So I looked at the draft version used in the link I put in the commit message. It says (in 6.2.4:2), ``` The lifetime of an object is the portion of program execution during which storage is guaranteed to be reserved for it. An object exists, has a constant address, and retains its last-stored value throughout its lifetime. If an object is referred to outside of its lifetime, the behavior is undefined. The value of a pointer becomes indeterminate when the object it points to (or just past) reaches the end of its lifetime. ``` I couldn't find the definition of lifetime over a dynamically allocated object in the draft of C standard. I refer to this link (https://en.cppreference.com/w/c/language/lifetime) which suggests that the lifetime of an allocated object ends after the deallocation function is called upon it. I think maybe the more problematic issue is that the value of a freed pointer is intermediate. In another section of the same draft I found the following: J.2 Undefined behavior [ ... ] The value of a pointer that refers to space deallocated by a call to the free or realloc function is used (7.22.3). Since the C standard explicitly refers to free() and realloc(), does that mean that that statement about undefined behavior does not apply to munmap() (for user space code) nor to kfree() (for kernel code)? Bart.
Re: [PATCH] cxgb4: fix undefined behavior in mem.c
Yes, why wouldn't they be real bugs? I was simply pointing out the irrational
conclusion if the C standard is strictly applied to kernel code.
I think the spirit of the C standard is that one shouldn't rely on the
assumption that the value of a freed pointer does not change, even though in
practice any compiler developers probably won't bother to implement the logic to
change the pointer value or optimizations leveraging it even they are allowed
to. In other words, the original code may be a little bit problematic in the
spirit of the C standard whereas the patch simply makes it totally valid.
If it can be finally submitted, that would be great. If not, I'm totally fine.
Shaobo
On 2019/3/1 7:26, Doug Ledford wrote:
On Thu, 2019-02-28 at 16:57 -0700, Shaobo He wrote:
Good catch. But if we agree on that memory management functions are those
specified by the C standard, would it be OK to ignore so-called use after free
or double free bugs for the kernel as C standard does not apply to kfree?
No, most kernel use-after-free bugs are real bugs. This one might be
technically a bug by certain readings of the standard, but it's a non-
issue. Real use-after-free bugs don't just look at the value of a local
stack variable to get the memory's old address (which is what this does,
and the same could be achieved and be totally in spec by doing this:
old_ptr = mhp;
kfree(mhp);
pr_debug("%p\n", old_ptr);)
Real use after free things would actually dereference the pointer to
either read or write from the old memory region. That leads to data
corruption or kernel data leaks. Plus, in this case, the purpose of
printing the literal value of mhp is simply to provide a unique name for
tracing purposes. Since kfree() doesn't alter the local stack variable,
the name is still present in the local stack variable at the point you
call pr_debug().
It could be fixed. It's not like this patch is wrong. But I wouldn't
submit it this late in the -rc cycle, I'd just take it for next.
On 2/28/19 4:33 PM, Bart Van Assche wrote:
On Thu, 2019-02-28 at 16:18 -0700, Shaobo He wrote:
I can't afford a pdf version of the C standard. So I looked at the draft version
used in the link I put in the commit message. It says (in 6.2.4:2),
```
The lifetime of an object is the portion of program execution during which
storage is guaranteed to be reserved for it. An object exists, has a constant
address, and retains its last-stored value throughout its lifetime. If an object
is referred to outside of its lifetime, the behavior is undefined. The value of
a pointer becomes indeterminate when the object it points to (or just past)
reaches the end of its lifetime.
```
I couldn't find the definition of lifetime over a dynamically allocated object
in the draft of C standard. I refer to this link
(https://en.cppreference.com/w/c/language/lifetime) which suggests that the
lifetime of an allocated object ends after the deallocation function is called
upon it.
I think maybe the more problematic issue is that the value of a freed pointer is
intermediate.
In another section of the same draft I found the following:
J.2 Undefined behavior [ ... ] The value of a pointer that refers to space
deallocated by a call to the free or realloc function is used (7.22.3).
Since the C standard explicitly refers to free() and realloc(), does that
mean that that statement about undefined behavior does not apply to munmap()
(for user space code) nor to kfree() (for kernel code)?
Bart.
[PATCH] Remove deductively redundant NULL pointer checks
The fixes included in this commit essentially removes NULL pointer
checks on the return values of function `get_queue_ctx` as well as
`v4l2_m2m_get_vq` defined in file v4l2-mem2mem.c.
Function `get_queue_ctx` is very unlikely to return a NULL pointer
because its return value is an address composed of the base address
pointed by `m2m_ctx` and an offset of field `out_q_ctx` or `cap_q_ctx`.
Since the offset of either field is not 0, for the return value to be
NULL, pointer `m2m_ctx` must be a very large unsigned value such that
its addition to the offset overflows to NULL which may be undefined
according to this post:
https://wdtz.org/catching-pointer-overflow-bugs.html. Moreover, even if
`m2m_ctx` is NULL, the return value cannot be NULL, either. Therefore, I
think it is reasonable to conclude that the return value of function
`get_queue_ctx` cannot be NULL.
Given the return values of `get_queue_ctx` not being NULL, we can follow
a similar reasoning to conclude that the return value of
`v4l2_mem_get_vq` cannot be NULL since its return value is the same
address as the return value of `get_queue_ctx`. Therefore, this patch
also removes NULL pointer checks on the return values of
`v4l2_mem_get_vq`.
Signed-off-by: Shaobo He
---
drivers/media/platform/coda/coda-common.c | 4
drivers/media/platform/imx-pxp.c | 7 ---
drivers/media/platform/m2m-deinterlace.c | 7 ---
drivers/media/platform/mtk-jpeg/mtk_jpeg_core.c| 7 ---
drivers/media/platform/mtk-vcodec/mtk_vcodec_dec.c | 7 ---
drivers/media/platform/mtk-vcodec/mtk_vcodec_enc.c | 13 -
drivers/media/platform/mx2_emmaprp.c | 7 ---
drivers/media/platform/rcar_fdp1.c | 3 ---
drivers/media/platform/rcar_jpu.c | 8
drivers/media/platform/rockchip/rga/rga.c | 4
drivers/media/platform/s5p-g2d/g2d.c | 4
drivers/media/platform/s5p-jpeg/jpeg-core.c| 7 ---
drivers/media/platform/sh_veu.c| 2 --
drivers/media/platform/ti-vpe/vpe.c| 7 ---
drivers/media/platform/vicodec/vicodec-core.c | 5 -
drivers/media/platform/vim2m.c | 9 -
drivers/media/v4l2-core/v4l2-mem2mem.c | 4
17 files changed, 105 deletions(-)
diff --git a/drivers/media/platform/coda/coda-common.c
b/drivers/media/platform/coda/coda-common.c
index 7518f01..ee1e05b 100644
--- a/drivers/media/platform/coda/coda-common.c
+++ b/drivers/media/platform/coda/coda-common.c
@@ -696,8 +696,6 @@ static int coda_s_fmt(struct coda_ctx *ctx, struct
v4l2_format *f,
struct vb2_queue *vq;
vq = v4l2_m2m_get_vq(ctx->fh.m2m_ctx, f->type);
- if (!vq)
- return -EINVAL;
q_data = get_q_data(ctx, f->type);
if (!q_data)
@@ -817,8 +815,6 @@ static int coda_s_fmt_vid_out(struct file *file, void *priv,
ctx->quantization = f->fmt.pix.quantization;
dst_vq = v4l2_m2m_get_vq(ctx->fh.m2m_ctx, V4L2_BUF_TYPE_VIDEO_CAPTURE);
- if (!dst_vq)
- return -EINVAL;
/*
* Setting the capture queue format is not possible while the capture
diff --git a/drivers/media/platform/imx-pxp.c b/drivers/media/platform/imx-pxp.c
index c1c2554..d079b3c 100644
--- a/drivers/media/platform/imx-pxp.c
+++ b/drivers/media/platform/imx-pxp.c
@@ -1071,13 +1071,8 @@ static int pxp_enum_fmt_vid_out(struct file *file, void
*priv,
static int pxp_g_fmt(struct pxp_ctx *ctx, struct v4l2_format *f)
{
- struct vb2_queue *vq;
struct pxp_q_data *q_data;
- vq = v4l2_m2m_get_vq(ctx->fh.m2m_ctx, f->type);
- if (!vq)
- return -EINVAL;
-
q_data = get_q_data(ctx, f->type);
f->fmt.pix.width= q_data->width;
@@ -1220,8 +1215,6 @@ static int pxp_s_fmt(struct pxp_ctx *ctx, struct
v4l2_format *f)
struct vb2_queue *vq;
vq = v4l2_m2m_get_vq(ctx->fh.m2m_ctx, f->type);
- if (!vq)
- return -EINVAL;
q_data = get_q_data(ctx, f->type);
if (!q_data)
diff --git a/drivers/media/platform/m2m-deinterlace.c
b/drivers/media/platform/m2m-deinterlace.c
index c62e598..df03ffd 100644
--- a/drivers/media/platform/m2m-deinterlace.c
+++ b/drivers/media/platform/m2m-deinterlace.c
@@ -497,13 +497,8 @@ static int vidioc_enum_fmt_vid_out(struct file *file, void
*priv,
static int vidioc_g_fmt(struct deinterlace_ctx *ctx, struct v4l2_format *f)
{
- struct vb2_queue *vq;
struct deinterlace_q_data *q_data;
- vq = v4l2_m2m_get_vq(ctx->m2m_ctx, f->type);
- if (!vq)
- return -EINVAL;
-
q_data = get_q_data(f->type);
f->fmt.pix.width= q_data->width;
@@ -598,8 +593,6 @@ static int vidioc_s_fmt(struct deinterlace_ctx *ctx, struct
v4l2_format *f)
struct vb2_queue
[PATCH] Remove deductively redundant NULL pointer checks
The fixes included in this commit essentially removes NULL pointer
checks on the return values of function `get_queue_ctx` as well as
`v4l2_m2m_get_vq` defined in file v4l2-mem2mem.c.
Function `get_queue_ctx` is very unlikely to return a NULL pointer
because its return value is an address composed of the base address
pointed by `m2m_ctx` and an offset of field `out_q_ctx` or `cap_q_ctx`.
Since the offset of either field is not 0, for the return value to be
NULL, pointer `m2m_ctx` must be a very large unsigned value such that
its addition to the offset overflows to NULL which may be undefined
according to this post:
https://wdtz.org/catching-pointer-overflow-bugs.html. Moreover, even if
`m2m_ctx` is NULL, the return value cannot be NULL, either. Therefore, I
think it is reasonable to conclude that the return value of function
`get_queue_ctx` cannot be NULL.
Given the return values of `get_queue_ctx` not being NULL, we can follow
a similar reasoning to conclude that the return value of
`v4l2_mem_get_vq` cannot be NULL since its return value is the same
address as the return value of `get_queue_ctx`. Therefore, this patch
also removes NULL pointer checks on the return values of
`v4l2_mem_get_vq`.
Signed-off-by: Shaobo He
---
drivers/media/platform/coda/coda-common.c | 4
drivers/media/platform/imx-pxp.c | 7 ---
drivers/media/platform/m2m-deinterlace.c | 7 ---
drivers/media/platform/mtk-jpeg/mtk_jpeg_core.c| 7 ---
drivers/media/platform/mtk-vcodec/mtk_vcodec_dec.c | 7 ---
drivers/media/platform/mtk-vcodec/mtk_vcodec_enc.c | 13 -
drivers/media/platform/mx2_emmaprp.c | 7 ---
drivers/media/platform/rcar_fdp1.c | 3 ---
drivers/media/platform/rcar_jpu.c | 8
drivers/media/platform/rockchip/rga/rga.c | 4
drivers/media/platform/s5p-g2d/g2d.c | 4
drivers/media/platform/s5p-jpeg/jpeg-core.c| 7 ---
drivers/media/platform/sh_veu.c| 2 --
drivers/media/platform/ti-vpe/vpe.c| 7 ---
drivers/media/platform/vicodec/vicodec-core.c | 5 -
drivers/media/platform/vim2m.c | 7 ---
drivers/media/v4l2-core/v4l2-mem2mem.c | 4
17 files changed, 103 deletions(-)
diff --git a/drivers/media/platform/coda/coda-common.c
b/drivers/media/platform/coda/coda-common.c
index 7518f01..ee1e05b 100644
--- a/drivers/media/platform/coda/coda-common.c
+++ b/drivers/media/platform/coda/coda-common.c
@@ -696,8 +696,6 @@ static int coda_s_fmt(struct coda_ctx *ctx, struct
v4l2_format *f,
struct vb2_queue *vq;
vq = v4l2_m2m_get_vq(ctx->fh.m2m_ctx, f->type);
- if (!vq)
- return -EINVAL;
q_data = get_q_data(ctx, f->type);
if (!q_data)
@@ -817,8 +815,6 @@ static int coda_s_fmt_vid_out(struct file *file, void *priv,
ctx->quantization = f->fmt.pix.quantization;
dst_vq = v4l2_m2m_get_vq(ctx->fh.m2m_ctx, V4L2_BUF_TYPE_VIDEO_CAPTURE);
- if (!dst_vq)
- return -EINVAL;
/*
* Setting the capture queue format is not possible while the capture
diff --git a/drivers/media/platform/imx-pxp.c b/drivers/media/platform/imx-pxp.c
index c1c2554..d079b3c 100644
--- a/drivers/media/platform/imx-pxp.c
+++ b/drivers/media/platform/imx-pxp.c
@@ -1071,13 +1071,8 @@ static int pxp_enum_fmt_vid_out(struct file *file, void
*priv,
static int pxp_g_fmt(struct pxp_ctx *ctx, struct v4l2_format *f)
{
- struct vb2_queue *vq;
struct pxp_q_data *q_data;
- vq = v4l2_m2m_get_vq(ctx->fh.m2m_ctx, f->type);
- if (!vq)
- return -EINVAL;
-
q_data = get_q_data(ctx, f->type);
f->fmt.pix.width= q_data->width;
@@ -1220,8 +1215,6 @@ static int pxp_s_fmt(struct pxp_ctx *ctx, struct
v4l2_format *f)
struct vb2_queue *vq;
vq = v4l2_m2m_get_vq(ctx->fh.m2m_ctx, f->type);
- if (!vq)
- return -EINVAL;
q_data = get_q_data(ctx, f->type);
if (!q_data)
diff --git a/drivers/media/platform/m2m-deinterlace.c
b/drivers/media/platform/m2m-deinterlace.c
index c62e598..df03ffd 100644
--- a/drivers/media/platform/m2m-deinterlace.c
+++ b/drivers/media/platform/m2m-deinterlace.c
@@ -497,13 +497,8 @@ static int vidioc_enum_fmt_vid_out(struct file *file, void
*priv,
static int vidioc_g_fmt(struct deinterlace_ctx *ctx, struct v4l2_format *f)
{
- struct vb2_queue *vq;
struct deinterlace_q_data *q_data;
- vq = v4l2_m2m_get_vq(ctx->m2m_ctx, f->type);
- if (!vq)
- return -EINVAL;
-
q_data = get_q_data(f->type);
f->fmt.pix.width= q_data->width;
@@ -598,8 +593,6 @@ static int vidioc_s_fmt(struct deinterlace_ctx *ctx, struct
v4l2_format *f)
struct vb2_queue
