* [PATCH] RDMA/mlx5: Use memset_after() to zero struct mlx5_ib_mr
@ 2021-11-18 20:31 Kees Cook
2021-11-21 13:54 ` Leon Romanovsky
0 siblings, 1 reply; 7+ messages in thread
From: Kees Cook @ 2021-11-18 20:31 UTC (permalink / raw)
To: Leon Romanovsky
Cc: Kees Cook, Doug Ledford, Jason Gunthorpe, linux-kernel,
linux-rdma, linux-hardening
In preparation for FORTIFY_SOURCE performing compile-time and run-time
field bounds checking for memset(), avoid intentionally writing across
neighboring fields.
Use memset_after() to zero the end of struct mlx5_ib_mr that should
be initialized.
Signed-off-by: Kees Cook <keescook@chromium.org>
---
drivers/infiniband/hw/mlx5/mlx5_ib.h | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/drivers/infiniband/hw/mlx5/mlx5_ib.h b/drivers/infiniband/hw/mlx5/mlx5_ib.h
index e636e954f6bf..af94c9fe8753 100644
--- a/drivers/infiniband/hw/mlx5/mlx5_ib.h
+++ b/drivers/infiniband/hw/mlx5/mlx5_ib.h
@@ -665,8 +665,7 @@ struct mlx5_ib_mr {
/* User MR data */
struct mlx5_cache_ent *cache_ent;
struct ib_umem *umem;
-
- /* This is zero'd when the MR is allocated */
+ /* Everything after umem is zero'd when the MR is allocated */
union {
/* Used only while the MR is in the cache */
struct {
@@ -718,7 +717,7 @@ struct mlx5_ib_mr {
/* Zero the fields in the mr that are variant depending on usage */
static inline void mlx5_clear_mr(struct mlx5_ib_mr *mr)
{
- memset(mr->out, 0, sizeof(*mr) - offsetof(struct mlx5_ib_mr, out));
+ memset_after(mr, 0, umem);
}
static inline bool is_odp_mr(struct mlx5_ib_mr *mr)
--
2.30.2
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] RDMA/mlx5: Use memset_after() to zero struct mlx5_ib_mr
2021-11-18 20:31 [PATCH] RDMA/mlx5: Use memset_after() to zero struct mlx5_ib_mr Kees Cook
@ 2021-11-21 13:54 ` Leon Romanovsky
2021-12-07 18:47 ` Jason Gunthorpe
0 siblings, 1 reply; 7+ messages in thread
From: Leon Romanovsky @ 2021-11-21 13:54 UTC (permalink / raw)
To: Kees Cook
Cc: Doug Ledford, Jason Gunthorpe, linux-kernel, linux-rdma,
linux-hardening
On Thu, Nov 18, 2021 at 12:31:38PM -0800, Kees Cook wrote:
> In preparation for FORTIFY_SOURCE performing compile-time and run-time
> field bounds checking for memset(), avoid intentionally writing across
> neighboring fields.
>
> Use memset_after() to zero the end of struct mlx5_ib_mr that should
> be initialized.
>
> Signed-off-by: Kees Cook <keescook@chromium.org>
> ---
> drivers/infiniband/hw/mlx5/mlx5_ib.h | 5 ++---
> 1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/infiniband/hw/mlx5/mlx5_ib.h b/drivers/infiniband/hw/mlx5/mlx5_ib.h
> index e636e954f6bf..af94c9fe8753 100644
> --- a/drivers/infiniband/hw/mlx5/mlx5_ib.h
> +++ b/drivers/infiniband/hw/mlx5/mlx5_ib.h
> @@ -665,8 +665,7 @@ struct mlx5_ib_mr {
> /* User MR data */
> struct mlx5_cache_ent *cache_ent;
> struct ib_umem *umem;
> -
> - /* This is zero'd when the MR is allocated */
> + /* Everything after umem is zero'd when the MR is allocated */
> union {
> /* Used only while the MR is in the cache */
> struct {
> @@ -718,7 +717,7 @@ struct mlx5_ib_mr {
> /* Zero the fields in the mr that are variant depending on usage */
> static inline void mlx5_clear_mr(struct mlx5_ib_mr *mr)
> {
> - memset(mr->out, 0, sizeof(*mr) - offsetof(struct mlx5_ib_mr, out));
> + memset_after(mr, 0, umem);
I think that it is not equivalent change and you need "memset_after(mr, 0, cache_ent);"
to clear umem pointer too.
> }
>
> static inline bool is_odp_mr(struct mlx5_ib_mr *mr)
> --
> 2.30.2
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] RDMA/mlx5: Use memset_after() to zero struct mlx5_ib_mr
2021-11-21 13:54 ` Leon Romanovsky
@ 2021-12-07 18:47 ` Jason Gunthorpe
2021-12-07 19:41 ` Kees Cook
0 siblings, 1 reply; 7+ messages in thread
From: Jason Gunthorpe @ 2021-12-07 18:47 UTC (permalink / raw)
To: Leon Romanovsky, Kees Cook; +Cc: linux-kernel, linux-rdma, linux-hardening
On Sun, Nov 21, 2021 at 03:54:55PM +0200, Leon Romanovsky wrote:
> On Thu, Nov 18, 2021 at 12:31:38PM -0800, Kees Cook wrote:
> > In preparation for FORTIFY_SOURCE performing compile-time and run-time
> > field bounds checking for memset(), avoid intentionally writing across
> > neighboring fields.
> >
> > Use memset_after() to zero the end of struct mlx5_ib_mr that should
> > be initialized.
> >
> > Signed-off-by: Kees Cook <keescook@chromium.org>
> > drivers/infiniband/hw/mlx5/mlx5_ib.h | 5 ++---
> > 1 file changed, 2 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/infiniband/hw/mlx5/mlx5_ib.h b/drivers/infiniband/hw/mlx5/mlx5_ib.h
> > index e636e954f6bf..af94c9fe8753 100644
> > +++ b/drivers/infiniband/hw/mlx5/mlx5_ib.h
> > @@ -665,8 +665,7 @@ struct mlx5_ib_mr {
> > /* User MR data */
> > struct mlx5_cache_ent *cache_ent;
> > struct ib_umem *umem;
> > -
> > - /* This is zero'd when the MR is allocated */
> > + /* Everything after umem is zero'd when the MR is allocated */
> > union {
> > /* Used only while the MR is in the cache */
> > struct {
> > @@ -718,7 +717,7 @@ struct mlx5_ib_mr {
> > /* Zero the fields in the mr that are variant depending on usage */
> > static inline void mlx5_clear_mr(struct mlx5_ib_mr *mr)
> > {
> > - memset(mr->out, 0, sizeof(*mr) - offsetof(struct mlx5_ib_mr, out));
> > + memset_after(mr, 0, umem);
>
> I think that it is not equivalent change and you need "memset_after(mr, 0, cache_ent);"
> to clear umem pointer too.
Kees?
Jason
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] RDMA/mlx5: Use memset_after() to zero struct mlx5_ib_mr
2021-12-07 18:47 ` Jason Gunthorpe
@ 2021-12-07 19:41 ` Kees Cook
2021-12-07 19:45 ` Jason Gunthorpe
0 siblings, 1 reply; 7+ messages in thread
From: Kees Cook @ 2021-12-07 19:41 UTC (permalink / raw)
To: Jason Gunthorpe
Cc: Leon Romanovsky, linux-kernel, linux-rdma, linux-hardening
On Tue, Dec 07, 2021 at 02:47:29PM -0400, Jason Gunthorpe wrote:
> On Sun, Nov 21, 2021 at 03:54:55PM +0200, Leon Romanovsky wrote:
> > On Thu, Nov 18, 2021 at 12:31:38PM -0800, Kees Cook wrote:
> > > In preparation for FORTIFY_SOURCE performing compile-time and run-time
> > > field bounds checking for memset(), avoid intentionally writing across
> > > neighboring fields.
> > >
> > > Use memset_after() to zero the end of struct mlx5_ib_mr that should
> > > be initialized.
> > >
> > > Signed-off-by: Kees Cook <keescook@chromium.org>
> > > drivers/infiniband/hw/mlx5/mlx5_ib.h | 5 ++---
> > > 1 file changed, 2 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/drivers/infiniband/hw/mlx5/mlx5_ib.h b/drivers/infiniband/hw/mlx5/mlx5_ib.h
> > > index e636e954f6bf..af94c9fe8753 100644
> > > +++ b/drivers/infiniband/hw/mlx5/mlx5_ib.h
> > > @@ -665,8 +665,7 @@ struct mlx5_ib_mr {
> > > /* User MR data */
> > > struct mlx5_cache_ent *cache_ent;
> > > struct ib_umem *umem;
> > > -
> > > - /* This is zero'd when the MR is allocated */
> > > + /* Everything after umem is zero'd when the MR is allocated */
> > > union {
> > > /* Used only while the MR is in the cache */
> > > struct {
> > > @@ -718,7 +717,7 @@ struct mlx5_ib_mr {
> > > /* Zero the fields in the mr that are variant depending on usage */
> > > static inline void mlx5_clear_mr(struct mlx5_ib_mr *mr)
> > > {
> > > - memset(mr->out, 0, sizeof(*mr) - offsetof(struct mlx5_ib_mr, out));
> > > + memset_after(mr, 0, umem);
> >
> > I think that it is not equivalent change and you need "memset_after(mr, 0, cache_ent);"
> > to clear umem pointer too.
>
> Kees?
Oops, sorry, I missed the ealrier reply!
I don't think that matches -- the original code wipes from the start of
"out" to the end of the struct. "out" is the first thing in the union
after "umem", so "umem" was not wiped before. I retained that behavior
("wipe everything after umem").
Am I misunderstanding the desired behavior here?
Thanks!
--
Kees Cook
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] RDMA/mlx5: Use memset_after() to zero struct mlx5_ib_mr
2021-12-07 19:41 ` Kees Cook
@ 2021-12-07 19:45 ` Jason Gunthorpe
2022-01-12 20:49 ` Kees Cook
0 siblings, 1 reply; 7+ messages in thread
From: Jason Gunthorpe @ 2021-12-07 19:45 UTC (permalink / raw)
To: Kees Cook; +Cc: Leon Romanovsky, linux-kernel, linux-rdma, linux-hardening
On Tue, Dec 07, 2021 at 11:41:07AM -0800, Kees Cook wrote:
> On Tue, Dec 07, 2021 at 02:47:29PM -0400, Jason Gunthorpe wrote:
> > On Sun, Nov 21, 2021 at 03:54:55PM +0200, Leon Romanovsky wrote:
> > > On Thu, Nov 18, 2021 at 12:31:38PM -0800, Kees Cook wrote:
> > > > In preparation for FORTIFY_SOURCE performing compile-time and run-time
> > > > field bounds checking for memset(), avoid intentionally writing across
> > > > neighboring fields.
> > > >
> > > > Use memset_after() to zero the end of struct mlx5_ib_mr that should
> > > > be initialized.
> > > >
> > > > Signed-off-by: Kees Cook <keescook@chromium.org>
> > > > drivers/infiniband/hw/mlx5/mlx5_ib.h | 5 ++---
> > > > 1 file changed, 2 insertions(+), 3 deletions(-)
> > > >
> > > > diff --git a/drivers/infiniband/hw/mlx5/mlx5_ib.h b/drivers/infiniband/hw/mlx5/mlx5_ib.h
> > > > index e636e954f6bf..af94c9fe8753 100644
> > > > +++ b/drivers/infiniband/hw/mlx5/mlx5_ib.h
> > > > @@ -665,8 +665,7 @@ struct mlx5_ib_mr {
> > > > /* User MR data */
> > > > struct mlx5_cache_ent *cache_ent;
> > > > struct ib_umem *umem;
> > > > -
> > > > - /* This is zero'd when the MR is allocated */
> > > > + /* Everything after umem is zero'd when the MR is allocated */
> > > > union {
> > > > /* Used only while the MR is in the cache */
> > > > struct {
> > > > @@ -718,7 +717,7 @@ struct mlx5_ib_mr {
> > > > /* Zero the fields in the mr that are variant depending on usage */
> > > > static inline void mlx5_clear_mr(struct mlx5_ib_mr *mr)
> > > > {
> > > > - memset(mr->out, 0, sizeof(*mr) - offsetof(struct mlx5_ib_mr, out));
> > > > + memset_after(mr, 0, umem);
> > >
> > > I think that it is not equivalent change and you need "memset_after(mr, 0, cache_ent);"
> > > to clear umem pointer too.
> >
> > Kees?
>
> Oops, sorry, I missed the ealrier reply!
>
> I don't think that matches -- the original code wipes from the start of
> "out" to the end of the struct. "out" is the first thing in the union
> after "umem", so "umem" was not wiped before. I retained that behavior
> ("wipe everything after umem").
>
> Am I misunderstanding the desired behavior here?
Ah, it is this patch:
commit f0ae4afe3d35e67db042c58a52909e06262b740f
Author: Alaa Hleihel <alaa@nvidia.com>
Date: Mon Nov 22 13:41:51 2021 +0200
RDMA/mlx5: Fix releasing unallocated memory in dereg MR flow
Which moved umem into the union that is causing the confusion
It hasn't quite made it to a rc release yet, so I suppose the answer
is to rebase this on that then it is as Leon says about cache_ent
Jason
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] RDMA/mlx5: Use memset_after() to zero struct mlx5_ib_mr
2021-12-07 19:45 ` Jason Gunthorpe
@ 2022-01-12 20:49 ` Kees Cook
2022-01-13 0:32 ` Jason Gunthorpe
0 siblings, 1 reply; 7+ messages in thread
From: Kees Cook @ 2022-01-12 20:49 UTC (permalink / raw)
To: Jason Gunthorpe
Cc: Leon Romanovsky, linux-kernel, linux-rdma, linux-hardening
On Tue, Dec 07, 2021 at 03:45:25PM -0400, Jason Gunthorpe wrote:
> On Tue, Dec 07, 2021 at 11:41:07AM -0800, Kees Cook wrote:
> > On Tue, Dec 07, 2021 at 02:47:29PM -0400, Jason Gunthorpe wrote:
> > > On Sun, Nov 21, 2021 at 03:54:55PM +0200, Leon Romanovsky wrote:
> > > > On Thu, Nov 18, 2021 at 12:31:38PM -0800, Kees Cook wrote:
> > > > > In preparation for FORTIFY_SOURCE performing compile-time and run-time
> > > > > field bounds checking for memset(), avoid intentionally writing across
> > > > > neighboring fields.
> > > > >
> > > > > Use memset_after() to zero the end of struct mlx5_ib_mr that should
> > > > > be initialized.
> > > > >
> > > > > Signed-off-by: Kees Cook <keescook@chromium.org>
> > > > > drivers/infiniband/hw/mlx5/mlx5_ib.h | 5 ++---
> > > > > 1 file changed, 2 insertions(+), 3 deletions(-)
> > > > >
> > > > > diff --git a/drivers/infiniband/hw/mlx5/mlx5_ib.h b/drivers/infiniband/hw/mlx5/mlx5_ib.h
> > > > > index e636e954f6bf..af94c9fe8753 100644
> > > > > +++ b/drivers/infiniband/hw/mlx5/mlx5_ib.h
> > > > > @@ -665,8 +665,7 @@ struct mlx5_ib_mr {
> > > > > /* User MR data */
> > > > > struct mlx5_cache_ent *cache_ent;
> > > > > struct ib_umem *umem;
> > > > > -
> > > > > - /* This is zero'd when the MR is allocated */
> > > > > + /* Everything after umem is zero'd when the MR is allocated */
> > > > > union {
> > > > > /* Used only while the MR is in the cache */
> > > > > struct {
> > > > > @@ -718,7 +717,7 @@ struct mlx5_ib_mr {
> > > > > /* Zero the fields in the mr that are variant depending on usage */
> > > > > static inline void mlx5_clear_mr(struct mlx5_ib_mr *mr)
> > > > > {
> > > > > - memset(mr->out, 0, sizeof(*mr) - offsetof(struct mlx5_ib_mr, out));
> > > > > + memset_after(mr, 0, umem);
> > > >
> > > > I think that it is not equivalent change and you need "memset_after(mr, 0, cache_ent);"
> > > > to clear umem pointer too.
> > >
> > > Kees?
> >
> > Oops, sorry, I missed the ealrier reply!
> >
> > I don't think that matches -- the original code wipes from the start of
> > "out" to the end of the struct. "out" is the first thing in the union
> > after "umem", so "umem" was not wiped before. I retained that behavior
> > ("wipe everything after umem").
> >
> > Am I misunderstanding the desired behavior here?
>
> Ah, it is this patch:
>
> commit f0ae4afe3d35e67db042c58a52909e06262b740f
> Author: Alaa Hleihel <alaa@nvidia.com>
> Date: Mon Nov 22 13:41:51 2021 +0200
>
> RDMA/mlx5: Fix releasing unallocated memory in dereg MR flow
>
> Which moved umem into the union that is causing the confusion
>
> It hasn't quite made it to a rc release yet, so I suppose the answer
> is to rebase this on that then it is as Leon says about cache_ent
The umem patch appears to have been reverted. Should I send an updated
patch for memset_after()? I think it would simply be:
diff --git a/drivers/infiniband/hw/mlx5/mlx5_ib.h b/drivers/infiniband/hw/mlx5/mlx5_ib.h
index e3c33be9c5a0..a58f69b19705 100644
--- a/drivers/infiniband/hw/mlx5/mlx5_ib.h
+++ b/drivers/infiniband/hw/mlx5/mlx5_ib.h
@@ -667,7 +667,7 @@ struct mlx5_ib_mr {
struct mlx5_cache_ent *cache_ent;
struct ib_umem *umem;
- /* This is zero'd when the MR is allocated */
+ /* Everything after umem is zero'd when MR allocated */
union {
/* Used only while the MR is in the cache */
struct {
@@ -719,7 +719,7 @@ struct mlx5_ib_mr {
/* Zero the fields in the mr that are variant depending on usage */
static inline void mlx5_clear_mr(struct mlx5_ib_mr *mr)
{
- memset(mr->out, 0, sizeof(*mr) - offsetof(struct mlx5_ib_mr, out));
+ memset_after(mr, 0, umem);
}
static inline bool is_odp_mr(struct mlx5_ib_mr *mr)
--
Kees Cook
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] RDMA/mlx5: Use memset_after() to zero struct mlx5_ib_mr
2022-01-12 20:49 ` Kees Cook
@ 2022-01-13 0:32 ` Jason Gunthorpe
0 siblings, 0 replies; 7+ messages in thread
From: Jason Gunthorpe @ 2022-01-13 0:32 UTC (permalink / raw)
To: Kees Cook; +Cc: Leon Romanovsky, linux-kernel, linux-rdma, linux-hardening
On Wed, Jan 12, 2022 at 12:49:13PM -0800, Kees Cook wrote:
> On Tue, Dec 07, 2021 at 03:45:25PM -0400, Jason Gunthorpe wrote:
> > On Tue, Dec 07, 2021 at 11:41:07AM -0800, Kees Cook wrote:
> > > On Tue, Dec 07, 2021 at 02:47:29PM -0400, Jason Gunthorpe wrote:
> > > > On Sun, Nov 21, 2021 at 03:54:55PM +0200, Leon Romanovsky wrote:
> > > > > On Thu, Nov 18, 2021 at 12:31:38PM -0800, Kees Cook wrote:
> > > > > > In preparation for FORTIFY_SOURCE performing compile-time and run-time
> > > > > > field bounds checking for memset(), avoid intentionally writing across
> > > > > > neighboring fields.
> > > > > >
> > > > > > Use memset_after() to zero the end of struct mlx5_ib_mr that should
> > > > > > be initialized.
> > > > > >
> > > > > > Signed-off-by: Kees Cook <keescook@chromium.org>
> > > > > > drivers/infiniband/hw/mlx5/mlx5_ib.h | 5 ++---
> > > > > > 1 file changed, 2 insertions(+), 3 deletions(-)
> > > > > >
> > > > > > diff --git a/drivers/infiniband/hw/mlx5/mlx5_ib.h b/drivers/infiniband/hw/mlx5/mlx5_ib.h
> > > > > > index e636e954f6bf..af94c9fe8753 100644
> > > > > > +++ b/drivers/infiniband/hw/mlx5/mlx5_ib.h
> > > > > > @@ -665,8 +665,7 @@ struct mlx5_ib_mr {
> > > > > > /* User MR data */
> > > > > > struct mlx5_cache_ent *cache_ent;
> > > > > > struct ib_umem *umem;
> > > > > > -
> > > > > > - /* This is zero'd when the MR is allocated */
> > > > > > + /* Everything after umem is zero'd when the MR is allocated */
> > > > > > union {
> > > > > > /* Used only while the MR is in the cache */
> > > > > > struct {
> > > > > > @@ -718,7 +717,7 @@ struct mlx5_ib_mr {
> > > > > > /* Zero the fields in the mr that are variant depending on usage */
> > > > > > static inline void mlx5_clear_mr(struct mlx5_ib_mr *mr)
> > > > > > {
> > > > > > - memset(mr->out, 0, sizeof(*mr) - offsetof(struct mlx5_ib_mr, out));
> > > > > > + memset_after(mr, 0, umem);
> > > > >
> > > > > I think that it is not equivalent change and you need "memset_after(mr, 0, cache_ent);"
> > > > > to clear umem pointer too.
> > > >
> > > > Kees?
> > >
> > > Oops, sorry, I missed the ealrier reply!
> > >
> > > I don't think that matches -- the original code wipes from the start of
> > > "out" to the end of the struct. "out" is the first thing in the union
> > > after "umem", so "umem" was not wiped before. I retained that behavior
> > > ("wipe everything after umem").
> > >
> > > Am I misunderstanding the desired behavior here?
> >
> > Ah, it is this patch:
> >
> > commit f0ae4afe3d35e67db042c58a52909e06262b740f
> > Author: Alaa Hleihel <alaa@nvidia.com>
> > Date: Mon Nov 22 13:41:51 2021 +0200
> >
> > RDMA/mlx5: Fix releasing unallocated memory in dereg MR flow
> >
> > Which moved umem into the union that is causing the confusion
> >
> > It hasn't quite made it to a rc release yet, so I suppose the answer
> > is to rebase this on that then it is as Leon says about cache_ent
>
> The umem patch appears to have been reverted. Should I send an updated
> patch for memset_after()? I think it would simply be:
No, I'll fix it in the merge. It is still correct to zero everything
after cache_ent
Thanks,
Jason
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2022-01-13 0:32 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2021-11-18 20:31 [PATCH] RDMA/mlx5: Use memset_after() to zero struct mlx5_ib_mr Kees Cook
2021-11-21 13:54 ` Leon Romanovsky
2021-12-07 18:47 ` Jason Gunthorpe
2021-12-07 19:41 ` Kees Cook
2021-12-07 19:45 ` Jason Gunthorpe
2022-01-12 20:49 ` Kees Cook
2022-01-13 0:32 ` Jason Gunthorpe
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).