linux-kernel - Re: [PATCH] RDMA/mlx5: Use memset_after() to zero struct mlx5_ib

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 7 Dec 2021 11:41:07 -0800
From:   Kees Cook <keescook@...omium.org>
To:     Jason Gunthorpe <jgg@...dia.com>
Cc:     Leon Romanovsky <leon@...nel.org>, linux-kernel@...r.kernel.org,
        linux-rdma@...r.kernel.org, linux-hardening@...r.kernel.org
Subject: Re: [PATCH] RDMA/mlx5: Use memset_after() to zero struct mlx5_ib_mr

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@...omium.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