linux-kernel - RE: [PATCH][next] mei: Avoid the use of one-element arrays

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, 14 Jul 2020 19:57:51 +0000
From:   "Winkler, Tomas" <tomas.winkler@...el.com>
To:     "Gustavo A. R. Silva" <gustavoars@...nel.org>
CC:     Arnd Bergmann <arnd@...db.de>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: RE: [PATCH][next] mei: Avoid the use of one-element arrays

> 
> On Tue, Jul 14, 2020 at 05:54:32PM +0000, Winkler, Tomas wrote:
> > >
> > > There is a regular need in the kernel to provide a way to declare
> > > having a dynamically sized set of trailing elements in a structure.
> > > Kernel code should always use “flexible array members”[1] for these
> > > cases or, as in this particular case, replace the one-element array
> > > with a simple value type u8 reserved once this is just a placeholder
> > > for alignment. The older style of one-element or zero-length arrays
> should no longer be used[2].
> > >
> > > Also, while there, use the preferred form for passing a size of a struct.
> > > The alternative form where struct name is spelled out hurts
> > > readability and introduces an opportunity for a bug when the
> > > variable type is changed but the corresponding sizeof that is passed as
> argument is not.
> > >
> > > [1] https://en.wikipedia.org/wiki/Flexible_array_member
> > > [2] https://github.com/KSPP/linux/issues/79
> > >
> > > Signed-off-by: Gustavo A. R. Silva <gustavoars@...nel.org>
> > I'm okay with the patch but in this case the description is a bit off.
> > In this case there was no intention for a flexible arrays its just a reserved
> field.
> >
> 
> The reserved field is actually mentioned in the description:
> 
> "... or, as in this particular case, replace the one-element array with a simple
> value type u8 reserved once this is just a placeholder for alignment."

Right, but it looks not connected to overall context, it looks like not very clean reuse of a commit message.
I would say that this reserved[1] rather had confused the detection scripts you are using for the  cleanup you are doing. 
Again, I'm okay with the patch, but if you can  reword the commit message it would be even more okay.

> 
> Thanks
> --
> Gustavo
> 
> > > ---
> > >  drivers/misc/mei/hbm.c | 4 ++--
> > >  drivers/misc/mei/hw.h  | 6 +++---
> > >  2 files changed, 5 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/drivers/misc/mei/hbm.c b/drivers/misc/mei/hbm.c index
> > > a44094cdbc36..f020d5594154 100644
> > > --- a/drivers/misc/mei/hbm.c
> > > +++ b/drivers/misc/mei/hbm.c
> > > @@ -408,14 +408,14 @@ static int mei_hbm_add_cl_resp(struct
> > > mei_device *dev, u8 addr, u8 status)  {
> > >  	struct mei_msg_hdr mei_hdr;
> > >  	struct hbm_add_client_response resp;
> > > -	const size_t len = sizeof(struct hbm_add_client_response);
> > > +	const size_t len = sizeof(resp);
> > >  	int ret;
> > >
> > >  	dev_dbg(dev->dev, "adding client response\n");
> > >
> > >  	mei_hbm_hdr(&mei_hdr, len);
> > >
> > > -	memset(&resp, 0, sizeof(struct hbm_add_client_response));
> > > +	memset(&resp, 0, len);
> > >  	resp.hbm_cmd = MEI_HBM_ADD_CLIENT_RES_CMD;
> > >  	resp.me_addr = addr;
> > >  	resp.status  = status;
> > > diff --git a/drivers/misc/mei/hw.h b/drivers/misc/mei/hw.h index
> > > b1a8d5ec88b3..8c0297f0e7f3 100644
> > > --- a/drivers/misc/mei/hw.h
> > > +++ b/drivers/misc/mei/hw.h
> > > @@ -346,13 +346,13 @@ struct hbm_add_client_request {
> > >   * @hbm_cmd: bus message command header
> > >   * @me_addr: address of the client in ME
> > >   * @status: if HBMS_SUCCESS then the client can now accept
> connections.
> > > - * @reserved: reserved
> > > + * @reserved: reserved for alignment.
> > >   */
> > >  struct hbm_add_client_response {
> > >  	u8 hbm_cmd;
> > >  	u8 me_addr;
> > >  	u8 status;
> > > -	u8 reserved[1];
> > > +	u8 reserved;
> > >  } __packed;
> > >
> > >  /**
> > > @@ -461,7 +461,7 @@ struct hbm_notification {
> > >  	u8 hbm_cmd;
> > >  	u8 me_addr;
> > >  	u8 host_addr;
> > > -	u8 reserved[1];
> > > +	u8 reserved;
> > >  } __packed;
> > >
> > >  /**
> > > --
> > > 2.27.0
> >