Our code analyzer reported a uaf.
In siw_alloc_mr, it calls siw_mr_add_mem(mr,..). In the implementation
of siw_mr_add_mem(), mem is assigned to mr->mem and then mem is freed
via kfree(mem) if xa_alloc_cyclic() failed. Here, mr->mem still point
to a freed object. After, the execution continue up to the err_out branch
of siw_alloc_mr, and the freed mr->mem is used in siw_mr_drop_mem(mr).
My patch moves "mr->mem = mem" behind the if (xa_alloc_cyclic(..)<0) {}
section, to avoid the uaf.
Fixes: 2251334dcac9e ("rdma/siw: application buffer management")
Signed-off-by: Lv Yunlong <[email protected]>
---
drivers/infiniband/sw/siw/siw_mem.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/infiniband/sw/siw/siw_mem.c b/drivers/infiniband/sw/siw/siw_mem.c
index 34a910cf0edb..96b38cfbb513 100644
--- a/drivers/infiniband/sw/siw/siw_mem.c
+++ b/drivers/infiniband/sw/siw/siw_mem.c
@@ -106,8 +106,6 @@ int siw_mr_add_mem(struct siw_mr *mr, struct ib_pd *pd, void *mem_obj,
mem->perms = rights & IWARP_ACCESS_MASK;
kref_init(&mem->ref);
- mr->mem = mem;
-
get_random_bytes(&next, 4);
next &= 0x00ffffff;
@@ -116,6 +114,8 @@ int siw_mr_add_mem(struct siw_mr *mr, struct ib_pd *pd, void *mem_obj,
kfree(mem);
return -ENOMEM;
}
+
+ mr->mem = mem;
/* Set the STag index part */
mem->stag = id << 8;
mr->base_mr.lkey = mr->base_mr.rkey = mem->stag;
--
2.25.1
On Sun, Apr 25, 2021 at 06:16:47PM -0700, Lv Yunlong wrote:
> Our code analyzer reported a uaf.
Can you please share more details about this "code analyzer"?
Thanks
>
> In siw_alloc_mr, it calls siw_mr_add_mem(mr,..). In the implementation
> of siw_mr_add_mem(), mem is assigned to mr->mem and then mem is freed
> via kfree(mem) if xa_alloc_cyclic() failed. Here, mr->mem still point
> to a freed object. After, the execution continue up to the err_out branch
> of siw_alloc_mr, and the freed mr->mem is used in siw_mr_drop_mem(mr).
>
> My patch moves "mr->mem = mem" behind the if (xa_alloc_cyclic(..)<0) {}
> section, to avoid the uaf.
>
> Fixes: 2251334dcac9e ("rdma/siw: application buffer management")
> Signed-off-by: Lv Yunlong <[email protected]>
> ---
> drivers/infiniband/sw/siw/siw_mem.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/infiniband/sw/siw/siw_mem.c b/drivers/infiniband/sw/siw/siw_mem.c
> index 34a910cf0edb..96b38cfbb513 100644
> --- a/drivers/infiniband/sw/siw/siw_mem.c
> +++ b/drivers/infiniband/sw/siw/siw_mem.c
> @@ -106,8 +106,6 @@ int siw_mr_add_mem(struct siw_mr *mr, struct ib_pd *pd, void *mem_obj,
> mem->perms = rights & IWARP_ACCESS_MASK;
> kref_init(&mem->ref);
>
> - mr->mem = mem;
> -
> get_random_bytes(&next, 4);
> next &= 0x00ffffff;
>
> @@ -116,6 +114,8 @@ int siw_mr_add_mem(struct siw_mr *mr, struct ib_pd *pd, void *mem_obj,
> kfree(mem);
> return -ENOMEM;
> }
> +
> + mr->mem = mem;
> /* Set the STag index part */
> mem->stag = id << 8;
> mr->base_mr.lkey = mr->base_mr.rkey = mem->stag;
> --
> 2.25.1
>
>
-----"Lv Yunlong" <[email protected]> wrote: -----
>To: [email protected], [email protected], [email protected]
>From: "Lv Yunlong" <[email protected]>
>Date: 04/26/2021 03:17AM
>Cc: [email protected], [email protected], "Lv
>Yunlong" <[email protected]>
>Subject: [EXTERNAL] [PATCH v2] rdma/siw: Fix a use after free in
>siw_alloc_mr
>
>Our code analyzer reported a uaf.
>
>In siw_alloc_mr, it calls siw_mr_add_mem(mr,..). In the
>implementation
>of siw_mr_add_mem(), mem is assigned to mr->mem and then mem is freed
>via kfree(mem) if xa_alloc_cyclic() failed. Here, mr->mem still point
>to a freed object. After, the execution continue up to the err_out
>branch
>of siw_alloc_mr, and the freed mr->mem is used in
>siw_mr_drop_mem(mr).
>
>My patch moves "mr->mem = mem" behind the if (xa_alloc_cyclic(..)<0)
>{}
>section, to avoid the uaf.
>
>Fixes: 2251334dcac9e ("rdma/siw: application buffer management")
>Signed-off-by: Lv Yunlong <[email protected]>
>---
> drivers/infiniband/sw/siw/siw_mem.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
>diff --git a/drivers/infiniband/sw/siw/siw_mem.c
>b/drivers/infiniband/sw/siw/siw_mem.c
>index 34a910cf0edb..96b38cfbb513 100644
>--- a/drivers/infiniband/sw/siw/siw_mem.c
>+++ b/drivers/infiniband/sw/siw/siw_mem.c
>@@ -106,8 +106,6 @@ int siw_mr_add_mem(struct siw_mr *mr, struct
>ib_pd *pd, void *mem_obj,
> mem->perms = rights & IWARP_ACCESS_MASK;
> kref_init(&mem->ref);
>
>- mr->mem = mem;
>-
> get_random_bytes(&next, 4);
> next &= 0x00ffffff;
>
>@@ -116,6 +114,8 @@ int siw_mr_add_mem(struct siw_mr *mr, struct
>ib_pd *pd, void *mem_obj,
> kfree(mem);
> return -ENOMEM;
> }
>+
>+ mr->mem = mem;
> /* Set the STag index part */
> mem->stag = id << 8;
> mr->base_mr.lkey = mr->base_mr.rkey = mem->stag;
>--
>2.25.1
>
>
>
Lv Yunlong, many thanks for catching, and thanks to
Leon for improving it.
Reviewed-by: Bernard Metzler <[email protected]>
> -----原始邮件-----
> 发件人: "Leon Romanovsky" <[email protected]>
> 发送时间: 2021-04-26 13:08:50 (星期一)
> 收件人: "Lv Yunlong" <[email protected]>
> 抄送: [email protected], [email protected], [email protected], [email protected], [email protected]
> 主题: Re: [PATCH v2] rdma/siw: Fix a use after free in siw_alloc_mr
>
> On Sun, Apr 25, 2021 at 06:16:47PM -0700, Lv Yunlong wrote:
> > Our code analyzer reported a uaf.
>
> Can you please share more details about this "code analyzer"?
>
> Thanks
>
> >
> > In siw_alloc_mr, it calls siw_mr_add_mem(mr,..). In the implementation
> > of siw_mr_add_mem(), mem is assigned to mr->mem and then mem is freed
> > via kfree(mem) if xa_alloc_cyclic() failed. Here, mr->mem still point
> > to a freed object. After, the execution continue up to the err_out branch
> > of siw_alloc_mr, and the freed mr->mem is used in siw_mr_drop_mem(mr).
> >
> > My patch moves "mr->mem = mem" behind the if (xa_alloc_cyclic(..)<0) {}
> > section, to avoid the uaf.
> >
> > Fixes: 2251334dcac9e ("rdma/siw: application buffer management")
> > Signed-off-by: Lv Yunlong <[email protected]>
> > ---
> > drivers/infiniband/sw/siw/siw_mem.c | 4 ++--
> > 1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/infiniband/sw/siw/siw_mem.c b/drivers/infiniband/sw/siw/siw_mem.c
> > index 34a910cf0edb..96b38cfbb513 100644
> > --- a/drivers/infiniband/sw/siw/siw_mem.c
> > +++ b/drivers/infiniband/sw/siw/siw_mem.c
> > @@ -106,8 +106,6 @@ int siw_mr_add_mem(struct siw_mr *mr, struct ib_pd *pd, void *mem_obj,
> > mem->perms = rights & IWARP_ACCESS_MASK;
> > kref_init(&mem->ref);
> >
> > - mr->mem = mem;
> > -
> > get_random_bytes(&next, 4);
> > next &= 0x00ffffff;
> >
> > @@ -116,6 +114,8 @@ int siw_mr_add_mem(struct siw_mr *mr, struct ib_pd *pd, void *mem_obj,
> > kfree(mem);
> > return -ENOMEM;
> > }
> > +
> > + mr->mem = mem;
> > /* Set the STag index part */
> > mem->stag = id << 8;
> > mr->base_mr.lkey = mr->base_mr.rkey = mem->stag;
> > --
> > 2.25.1
> >
> >
Yes, i would like to share some details about it although it still has no name.
We designed this analzyer by adopt a deep learning model to recognize the
memory allocation and deallocation in kernel first. And then use Clang Static
Analyzer to scan the kernel code.
On Sun, Apr 25, 2021 at 06:16:47PM -0700, Lv Yunlong wrote:
> Our code analyzer reported a uaf.
>
> In siw_alloc_mr, it calls siw_mr_add_mem(mr,..). In the implementation
> of siw_mr_add_mem(), mem is assigned to mr->mem and then mem is freed
> via kfree(mem) if xa_alloc_cyclic() failed. Here, mr->mem still point
> to a freed object. After, the execution continue up to the err_out branch
> of siw_alloc_mr, and the freed mr->mem is used in siw_mr_drop_mem(mr).
>
> My patch moves "mr->mem = mem" behind the if (xa_alloc_cyclic(..)<0) {}
> section, to avoid the uaf.
>
> Fixes: 2251334dcac9e ("rdma/siw: application buffer management")
> Signed-off-by: Lv Yunlong <[email protected]>
> Reviewed-by: Bernard Metzler <[email protected]>
> ---
> drivers/infiniband/sw/siw/siw_mem.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
Applied to for-next, thanks
Jason