LinuxLists.cc - [char-misc-next 0/6] mei: remove one-element arrays

2020-07-23 15:00:24

Subject: [char-misc-next 0/6] mei: remove one-element arrays

1. Remove one-element arrays from hw.h
2. Adjust to preferred method of getting size of structure
from memory allocation and movement. sizeof(*var)
instead of sizeof(struct some_struct)

Tomas Winkler (6):
mei: hbm: use sizeof of variable instead of struct type
mei: ioctl: use sizeof of variable instead of struct type
mei: bus: use sizeof of variable instead of struct type
mei: client: use sizeof of variable instead of struct type
mei: hw: use sizeof of variable instead of struct type
mei: hw: don't use one element arrays

drivers/misc/mei/bus-fixup.c | 23 ++++++-----
drivers/misc/mei/bus.c | 2 +-
drivers/misc/mei/client.c | 8 ++--
drivers/misc/mei/hbm.c | 74 ++++++++++++++++--------------------
drivers/misc/mei/hw-me.c | 5 +--
drivers/misc/mei/hw-txe.c | 5 +--
drivers/misc/mei/hw.h | 8 ++--
drivers/misc/mei/main.c | 6 +--
8 files changed, 59 insertions(+), 72 deletions(-)

--
2.25.4

2020-07-23 15:00:52

by Tomas Winkler

[permalink] [raw]

Subject: [char-misc-next 2/6] mei: ioctl: use sizeof of variable instead of struct type

Use sizeof(connect_data))) instead of
sizeof(struct mei_connect_client_data) when copying data
between user space and kernel.

There is a possibility of bug when variable type has changed but
corresponding struct passed to the sizeof has not.

Cc: Gustavo A. R. Silva <[email protected]>
Signed-off-by: Tomas Winkler <[email protected]>
---
drivers/misc/mei/main.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/misc/mei/main.c b/drivers/misc/mei/main.c
index f17297f2943d..05e6ad6d4d54 100644
--- a/drivers/misc/mei/main.c
+++ b/drivers/misc/mei/main.c
@@ -1,6 +1,6 @@
// SPDX-License-Identifier: GPL-2.0
/*
- * Copyright (c) 2003-2018, Intel Corporation. All rights reserved.
+ * Copyright (c) 2003-2020, Intel Corporation. All rights reserved.
* Intel Management Engine Interface (Intel MEI) Linux driver
*/

@@ -476,7 +476,7 @@ static long mei_ioctl(struct file *file, unsigned int cmd, unsigned long data)
case IOCTL_MEI_CONNECT_CLIENT:
dev_dbg(dev->dev, ": IOCTL_MEI_CONNECT_CLIENT.\n");
if (copy_from_user(&connect_data, (char __user *)data,
- sizeof(struct mei_connect_client_data))) {
+ sizeof(connect_data))) {
dev_dbg(dev->dev, "failed to copy data from userland\n");
rets = -EFAULT;
goto out;
@@ -488,7 +488,7 @@ static long mei_ioctl(struct file *file, unsigned int cmd, unsigned long data)

/* if all is ok, copying the data back to user. */
if (copy_to_user((char __user *)data, &connect_data,
- sizeof(struct mei_connect_client_data))) {
+ sizeof(connect_data))) {
dev_dbg(dev->dev, "failed to copy data to userland\n");
rets = -EFAULT;
goto out;
--
2.25.4

2020-07-23 15:00:54

by Tomas Winkler

[permalink] [raw]

Subject: [char-misc-next 3/6] mei: bus: use sizeof of variable instead of struct type

2020-07-23 15:03:10

by Tomas Winkler

[permalink] [raw]

Subject: [char-misc-next 4/6] mei: client: use sizeof of variable instead of struct type

There is a possibility of bug when variable type has changed but
corresponding struct passed to the sizeof has not.

Cc: Gustavo A. R. Silva <[email protected]>
Signed-off-by: Tomas Winkler <[email protected]>
---
drivers/misc/mei/client.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/misc/mei/client.c b/drivers/misc/mei/client.c
index b32c825a0945..2572887d99b6 100644
--- a/drivers/misc/mei/client.c
+++ b/drivers/misc/mei/client.c
@@ -1,6 +1,6 @@
// SPDX-License-Identifier: GPL-2.0
/*
- * Copyright (c) 2003-2019, Intel Corporation. All rights reserved.
+ * Copyright (c) 2003-2020, Intel Corporation. All rights reserved.
* Intel Management Engine Interface (Intel MEI) Linux driver
*/

@@ -369,7 +369,7 @@ static struct mei_cl_cb *mei_io_cb_init(struct mei_cl *cl,
{
struct mei_cl_cb *cb;

- cb = kzalloc(sizeof(struct mei_cl_cb), GFP_KERNEL);
+ cb = kzalloc(sizeof(*cb), GFP_KERNEL);
if (!cb)
return NULL;

@@ -552,7 +552,7 @@ int mei_cl_flush_queues(struct mei_cl *cl, const struct file *fp)
*/
static void mei_cl_init(struct mei_cl *cl, struct mei_device *dev)
{
- memset(cl, 0, sizeof(struct mei_cl));
+ memset(cl, 0, sizeof(*cl));
init_waitqueue_head(&cl->wait);
init_waitqueue_head(&cl->rx_wait);
init_waitqueue_head(&cl->tx_wait);
@@ -575,7 +575,7 @@ struct mei_cl *mei_cl_allocate(struct mei_device *dev)
{
struct mei_cl *cl;

- cl = kmalloc(sizeof(struct mei_cl), GFP_KERNEL);
+ cl = kmalloc(sizeof(*cl), GFP_KERNEL);
if (!cl)
return NULL;

--
2.25.4

2020-07-23 15:03:12

by Tomas Winkler

[permalink] [raw]

Subject: [char-misc-next 1/6] mei: hbm: use sizeof of variable instead of struct type

There is a possibility of bug when variable type has changed but
corresponding struct passed to the sizeof has not.

Cc: Gustavo A. R. Silva <[email protected]>
Signed-off-by: Tomas Winkler <[email protected]>
---
drivers/misc/mei/hbm.c | 74 ++++++++++++++++++------------------------
1 file changed, 32 insertions(+), 42 deletions(-)

diff --git a/drivers/misc/mei/hbm.c b/drivers/misc/mei/hbm.c
index a44094cdbc36..308caee86920 100644
--- a/drivers/misc/mei/hbm.c
+++ b/drivers/misc/mei/hbm.c
@@ -1,6 +1,6 @@
// SPDX-License-Identifier: GPL-2.0
/*
- * Copyright (c) 2003-2019, Intel Corporation. All rights reserved.
+ * Copyright (c) 2003-2020, Intel Corporation. All rights reserved.
* Intel Management Engine Interface (Intel MEI) Linux driver
*/
#include <linux/export.h>
@@ -257,22 +257,21 @@ int mei_hbm_start_wait(struct mei_device *dev)
int mei_hbm_start_req(struct mei_device *dev)
{
struct mei_msg_hdr mei_hdr;
- struct hbm_host_version_request start_req;
- const size_t len = sizeof(struct hbm_host_version_request);
+ struct hbm_host_version_request req;
int ret;

mei_hbm_reset(dev);

- mei_hbm_hdr(&mei_hdr, len);
+ mei_hbm_hdr(&mei_hdr, sizeof(req));

/* host start message */
- memset(&start_req, 0, len);
- start_req.hbm_cmd = HOST_START_REQ_CMD;
- start_req.host_version.major_version = HBM_MAJOR_VERSION;
- start_req.host_version.minor_version = HBM_MINOR_VERSION;
+ memset(&req, 0, sizeof(req));
+ req.hbm_cmd = HOST_START_REQ_CMD;
+ req.host_version.major_version = HBM_MAJOR_VERSION;
+ req.host_version.minor_version = HBM_MINOR_VERSION;

dev->hbm_state = MEI_HBM_IDLE;
- ret = mei_hbm_write_message(dev, &mei_hdr, &start_req);
+ ret = mei_hbm_write_message(dev, &mei_hdr, &req);
if (ret) {
dev_err(dev->dev, "version message write failed: ret = %d\n",
ret);
@@ -295,13 +294,12 @@ static int mei_hbm_dma_setup_req(struct mei_device *dev)
{
struct mei_msg_hdr mei_hdr;
struct hbm_dma_setup_request req;
- const size_t len = sizeof(struct hbm_dma_setup_request);
unsigned int i;
int ret;

- mei_hbm_hdr(&mei_hdr, len);
+ mei_hbm_hdr(&mei_hdr, sizeof(req));

- memset(&req, 0, len);
+ memset(&req, 0, sizeof(req));
req.hbm_cmd = MEI_HBM_DMA_SETUP_REQ_CMD;
for (i = 0; i < DMA_DSCR_NUM; i++) {
phys_addr_t paddr;
@@ -337,21 +335,19 @@ static int mei_hbm_dma_setup_req(struct mei_device *dev)
static int mei_hbm_enum_clients_req(struct mei_device *dev)
{
struct mei_msg_hdr mei_hdr;
- struct hbm_host_enum_request enum_req;
- const size_t len = sizeof(struct hbm_host_enum_request);
+ struct hbm_host_enum_request req;
int ret;

/* enumerate clients */
- mei_hbm_hdr(&mei_hdr, len);
+ mei_hbm_hdr(&mei_hdr, sizeof(req));

- memset(&enum_req, 0, len);
- enum_req.hbm_cmd = HOST_ENUM_REQ_CMD;
- enum_req.flags |= dev->hbm_f_dc_supported ?
- MEI_HBM_ENUM_F_ALLOW_ADD : 0;
- enum_req.flags |= dev->hbm_f_ie_supported ?
+ memset(&req, 0, sizeof(req));
+ req.hbm_cmd = HOST_ENUM_REQ_CMD;
+ req.flags |= dev->hbm_f_dc_supported ? MEI_HBM_ENUM_F_ALLOW_ADD : 0;
+ req.flags |= dev->hbm_f_ie_supported ?
MEI_HBM_ENUM_F_IMMEDIATE_ENUM : 0;

- ret = mei_hbm_write_message(dev, &mei_hdr, &enum_req);
+ ret = mei_hbm_write_message(dev, &mei_hdr, &req);
if (ret) {
dev_err(dev->dev, "enumeration request write failed: ret = %d.\n",
ret);
@@ -380,7 +376,7 @@ static int mei_hbm_me_cl_add(struct mei_device *dev,

mei_me_cl_rm_by_uuid(dev, uuid);

- me_cl = kzalloc(sizeof(struct mei_me_client), GFP_KERNEL);
+ me_cl = kzalloc(sizeof(*me_cl), GFP_KERNEL);
if (!me_cl)
return -ENOMEM;

@@ -408,14 +404,13 @@ 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);
int ret;

dev_dbg(dev->dev, "adding client response\n");

- mei_hbm_hdr(&mei_hdr, len);
+ mei_hbm_hdr(&mei_hdr, sizeof(resp));

- memset(&resp, 0, sizeof(struct hbm_add_client_response));
+ memset(&resp, 0, sizeof(resp));
resp.hbm_cmd = MEI_HBM_ADD_CLIENT_RES_CMD;
resp.me_addr = addr;
resp.status = status;
@@ -469,11 +464,10 @@ int mei_hbm_cl_notify_req(struct mei_device *dev,

struct mei_msg_hdr mei_hdr;
struct hbm_notification_request req;
- const size_t len = sizeof(struct hbm_notification_request);
int ret;

- mei_hbm_hdr(&mei_hdr, len);
- mei_hbm_cl_hdr(cl, MEI_HBM_NOTIFY_REQ_CMD, &req, len);
+ mei_hbm_hdr(&mei_hdr, sizeof(req));
+ mei_hbm_cl_hdr(cl, MEI_HBM_NOTIFY_REQ_CMD, &req, sizeof(req));

req.start = start;

@@ -580,8 +574,7 @@ static void mei_hbm_cl_notify(struct mei_device *dev,
static int mei_hbm_prop_req(struct mei_device *dev, unsigned long start_idx)
{
struct mei_msg_hdr mei_hdr;
- struct hbm_props_request prop_req;
- const size_t len = sizeof(struct hbm_props_request);
+ struct hbm_props_request req;
unsigned long addr;
int ret;

@@ -591,18 +584,17 @@ static int mei_hbm_prop_req(struct mei_device *dev, unsigned long start_idx)
if (addr == MEI_CLIENTS_MAX) {
dev->hbm_state = MEI_HBM_STARTED;
mei_host_client_init(dev);
-
return 0;
}

- mei_hbm_hdr(&mei_hdr, len);
+ mei_hbm_hdr(&mei_hdr, sizeof(req));

- memset(&prop_req, 0, sizeof(struct hbm_props_request));
+ memset(&req, 0, sizeof(req));

- prop_req.hbm_cmd = HOST_CLIENT_PROPERTIES_REQ_CMD;
- prop_req.me_addr = addr;
+ req.hbm_cmd = HOST_CLIENT_PROPERTIES_REQ_CMD;
+ req.me_addr = addr;

- ret = mei_hbm_write_message(dev, &mei_hdr, &prop_req);
+ ret = mei_hbm_write_message(dev, &mei_hdr, &req);
if (ret) {
dev_err(dev->dev, "properties request write failed: ret = %d\n",
ret);
@@ -628,15 +620,14 @@ int mei_hbm_pg(struct mei_device *dev, u8 pg_cmd)
{
struct mei_msg_hdr mei_hdr;
struct hbm_power_gate req;
- const size_t len = sizeof(struct hbm_power_gate);
int ret;

if (!dev->hbm_f_pg_supported)
return -EOPNOTSUPP;

- mei_hbm_hdr(&mei_hdr, len);
+ mei_hbm_hdr(&mei_hdr, sizeof(req));

- memset(&req, 0, len);
+ memset(&req, 0, sizeof(req));
req.hbm_cmd = pg_cmd;

ret = mei_hbm_write_message(dev, &mei_hdr, &req);
@@ -657,11 +648,10 @@ static int mei_hbm_stop_req(struct mei_device *dev)
{
struct mei_msg_hdr mei_hdr;
struct hbm_host_stop_request req;
- const size_t len = sizeof(struct hbm_host_stop_request);

- mei_hbm_hdr(&mei_hdr, len);
+ mei_hbm_hdr(&mei_hdr, sizeof(req));

- memset(&req, 0, len);
+ memset(&req, 0, sizeof(req));
req.hbm_cmd = HOST_STOP_REQ_CMD;
req.reason = DRIVER_STOP_REQUEST;

--
2.25.4

2020-07-23 15:03:33

by Tomas Winkler

[permalink] [raw]

Subject: [char-misc-next 6/6] mei: hw: don't use one element arrays

Replace the single element arrays with a simple value type u8 reserved,
even thought is is not used for dynamically sized trailing elements
it confuses the effort of replacing one-element arrays with
flexible arrays for that purpose.

Link: https://github.com/KSPP/linux/issues/79
Cc: Gustavo A. R. Silva <[email protected]>
Signed-off-by: Tomas Winkler <[email protected]>
---
drivers/misc/mei/hw.h | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/misc/mei/hw.h b/drivers/misc/mei/hw.h
index b1a8d5ec88b3..26fa92cb7f7a 100644
--- a/drivers/misc/mei/hw.h
+++ b/drivers/misc/mei/hw.h
@@ -1,6 +1,6 @@
/* SPDX-License-Identifier: GPL-2.0 */
/*
- * Copyright (c) 2003-2018, Intel Corporation. All rights reserved
+ * Copyright (c) 2003-2020, Intel Corporation. All rights reserved
* Intel Management Engine Interface (Intel MEI) Linux driver
*/

@@ -319,7 +319,7 @@ struct hbm_props_response {
u8 hbm_cmd;
u8 me_addr;
u8 status;
- u8 reserved[1];
+ u8 reserved;
struct mei_client_properties client_properties;
} __packed;

@@ -352,7 +352,7 @@ 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.25.4

2020-07-23 15:03:41

by Tomas Winkler

[permalink] [raw]

Subject: [char-misc-next 5/6] mei: hw: use sizeof of variable instead of struct type

Use sizeof(*dev) + sizeof(*hw) instead of
sizeof(struct mei_device) + sizeof(struct mei_me_hw)

There is a possibility of bug when variable type has changed but
corresponding struct passed to the sizeof has not.

Cc: Gustavo A. R. Silva <[email protected]>
Signed-off-by: Tomas Winkler <[email protected]>
---
drivers/misc/mei/hw-me.c | 5 ++---
drivers/misc/mei/hw-txe.c | 5 ++---
2 files changed, 4 insertions(+), 6 deletions(-)

diff --git a/drivers/misc/mei/hw-me.c b/drivers/misc/mei/hw-me.c
index c51d3da8f333..7692b69abcb5 100644
--- a/drivers/misc/mei/hw-me.c
+++ b/drivers/misc/mei/hw-me.c
@@ -1,6 +1,6 @@
// SPDX-License-Identifier: GPL-2.0
/*
- * Copyright (c) 2003-2019, Intel Corporation. All rights reserved.
+ * Copyright (c) 2003-2020, Intel Corporation. All rights reserved.
* Intel Management Engine Interface (Intel MEI) Linux driver
*/

@@ -1600,8 +1600,7 @@ struct mei_device *mei_me_dev_init(struct device *parent,
struct mei_me_hw *hw;
int i;

- dev = devm_kzalloc(parent, sizeof(struct mei_device) +
- sizeof(struct mei_me_hw), GFP_KERNEL);
+ dev = devm_kzalloc(parent, sizeof(*dev) + sizeof(*hw), GFP_KERNEL);
if (!dev)
return NULL;

diff --git a/drivers/misc/mei/hw-txe.c b/drivers/misc/mei/hw-txe.c
index 785b260b3ae9..a4e854b9b9e6 100644
--- a/drivers/misc/mei/hw-txe.c
+++ b/drivers/misc/mei/hw-txe.c
@@ -1,6 +1,6 @@
// SPDX-License-Identifier: GPL-2.0
/*
- * Copyright (c) 2013-2019, Intel Corporation. All rights reserved.
+ * Copyright (c) 2013-2020, Intel Corporation. All rights reserved.
* Intel Management Engine Interface (Intel MEI) Linux driver
*/

@@ -1201,8 +1201,7 @@ struct mei_device *mei_txe_dev_init(struct pci_dev *pdev)
struct mei_device *dev;
struct mei_txe_hw *hw;

- dev = devm_kzalloc(&pdev->dev, sizeof(struct mei_device) +
- sizeof(struct mei_txe_hw), GFP_KERNEL);
+ dev = devm_kzalloc(&pdev->dev, sizeof(*dev) + sizeof(*hw), GFP_KERNEL);
if (!dev)
return NULL;

--
2.25.4

2020-07-23 15:14:26

by Greg Kroah-Hartman

[permalink] [raw]

Subject: Re: [char-misc-next 1/6] mei: hbm: use sizeof of variable instead of struct type

On Thu, Jul 23, 2020 at 05:59:22PM +0300, Tomas Winkler wrote:
> There is a possibility of bug when variable type has changed but
> corresponding struct passed to the sizeof has not.
>
> Cc: Gustavo A. R. Silva <[email protected]>
> Signed-off-by: Tomas Winkler <[email protected]>
> ---
> drivers/misc/mei/hbm.c | 74 ++++++++++++++++++------------------------
> 1 file changed, 32 insertions(+), 42 deletions(-)

This doesn't apply to my tree as I've applied Gustavo's patch. Should I
revert that first?

thanks,

greg k-h

2020-07-23 16:46:24

by Gustavo A. R. Silva

[permalink] [raw]

Subject: Re: [char-misc-next 6/6] mei: hw: don't use one element arrays

On 7/23/20 09:59, Tomas Winkler wrote:
> Replace the single element arrays with a simple value type u8 reserved,
> even thought is is not used for dynamically sized trailing elements
> it confuses the effort of replacing one-element arrays with
> flexible arrays for that purpose.
>
> Link: https://github.com/KSPP/linux/issues/79
> Cc: Gustavo A. R. Silva <[email protected]>
> Signed-off-by: Tomas Winkler <[email protected]>

Reviewed-by: Gustavo A. R. Silva <[email protected]>

Thanks
--
Gustavo

> ---
> drivers/misc/mei/hw.h | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/misc/mei/hw.h b/drivers/misc/mei/hw.h
> index b1a8d5ec88b3..26fa92cb7f7a 100644
> --- a/drivers/misc/mei/hw.h
> +++ b/drivers/misc/mei/hw.h
> @@ -1,6 +1,6 @@
> /* SPDX-License-Identifier: GPL-2.0 */
> /*
> - * Copyright (c) 2003-2018, Intel Corporation. All rights reserved
> + * Copyright (c) 2003-2020, Intel Corporation. All rights reserved
> * Intel Management Engine Interface (Intel MEI) Linux driver
> */
>
> @@ -319,7 +319,7 @@ struct hbm_props_response {
> u8 hbm_cmd;
> u8 me_addr;
> u8 status;
> - u8 reserved[1];
> + u8 reserved;
> struct mei_client_properties client_properties;
> } __packed;
>
> @@ -352,7 +352,7 @@ 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;
>
> /**
>

2020-07-23 16:49:15

by Gustavo A. R. Silva

[permalink] [raw]

Subject: Re: [char-misc-next 5/6] mei: hw: use sizeof of variable instead of struct type

On 7/23/20 09:59, Tomas Winkler wrote:
> Use sizeof(*dev) + sizeof(*hw) instead of
> sizeof(struct mei_device) + sizeof(struct mei_me_hw)
>
> There is a possibility of bug when variable type has changed but
> corresponding struct passed to the sizeof has not.
>
> Cc: Gustavo A. R. Silva <[email protected]>
> Signed-off-by: Tomas Winkler <[email protected]>

Reviewed-by: Gustavo A. R. Silva <[email protected]>

Thanks
--
Gustavo

> ---
> drivers/misc/mei/hw-me.c | 5 ++---
> drivers/misc/mei/hw-txe.c | 5 ++---
> 2 files changed, 4 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/misc/mei/hw-me.c b/drivers/misc/mei/hw-me.c
> index c51d3da8f333..7692b69abcb5 100644
> --- a/drivers/misc/mei/hw-me.c
> +++ b/drivers/misc/mei/hw-me.c
> @@ -1,6 +1,6 @@
> // SPDX-License-Identifier: GPL-2.0
> /*
> - * Copyright (c) 2003-2019, Intel Corporation. All rights reserved.
> + * Copyright (c) 2003-2020, Intel Corporation. All rights reserved.
> * Intel Management Engine Interface (Intel MEI) Linux driver
> */
>
> @@ -1600,8 +1600,7 @@ struct mei_device *mei_me_dev_init(struct device *parent,
> struct mei_me_hw *hw;
> int i;
>
> - dev = devm_kzalloc(parent, sizeof(struct mei_device) +
> - sizeof(struct mei_me_hw), GFP_KERNEL);
> + dev = devm_kzalloc(parent, sizeof(*dev) + sizeof(*hw), GFP_KERNEL);
> if (!dev)
> return NULL;
>
> diff --git a/drivers/misc/mei/hw-txe.c b/drivers/misc/mei/hw-txe.c
> index 785b260b3ae9..a4e854b9b9e6 100644
> --- a/drivers/misc/mei/hw-txe.c
> +++ b/drivers/misc/mei/hw-txe.c
> @@ -1,6 +1,6 @@
> // SPDX-License-Identifier: GPL-2.0
> /*
> - * Copyright (c) 2013-2019, Intel Corporation. All rights reserved.
> + * Copyright (c) 2013-2020, Intel Corporation. All rights reserved.
> * Intel Management Engine Interface (Intel MEI) Linux driver
> */
>
> @@ -1201,8 +1201,7 @@ struct mei_device *mei_txe_dev_init(struct pci_dev *pdev)
> struct mei_device *dev;
> struct mei_txe_hw *hw;
>
> - dev = devm_kzalloc(&pdev->dev, sizeof(struct mei_device) +
> - sizeof(struct mei_txe_hw), GFP_KERNEL);
> + dev = devm_kzalloc(&pdev->dev, sizeof(*dev) + sizeof(*hw), GFP_KERNEL);
> if (!dev)
> return NULL;
>
>

2020-07-23 16:50:10

by Gustavo A. R. Silva

[permalink] [raw]

Subject: Re: [char-misc-next 4/6] mei: client: use sizeof of variable instead of struct type

2020-07-23 16:50:42

by Gustavo A. R. Silva

[permalink] [raw]

Subject: Re: [char-misc-next 3/6] mei: bus: use sizeof of variable instead of struct type

2020-07-23 16:51:35

by Gustavo A. R. Silva

[permalink] [raw]

Subject: Re: [char-misc-next 2/6] mei: ioctl: use sizeof of variable instead of struct type

On 7/23/20 09:59, Tomas Winkler wrote:
> Use sizeof(connect_data))) instead of
> sizeof(struct mei_connect_client_data) when copying data
> between user space and kernel.
>
> There is a possibility of bug when variable type has changed but
> corresponding struct passed to the sizeof has not.
>
> Cc: Gustavo A. R. Silva <[email protected]>
> Signed-off-by: Tomas Winkler <[email protected]>

Reviewed-by: Gustavo A. R. Silva <[email protected]>

Thanks
--
Gustavo

> ---
> drivers/misc/mei/main.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/misc/mei/main.c b/drivers/misc/mei/main.c
> index f17297f2943d..05e6ad6d4d54 100644
> --- a/drivers/misc/mei/main.c
> +++ b/drivers/misc/mei/main.c
> @@ -1,6 +1,6 @@
> // SPDX-License-Identifier: GPL-2.0
> /*
> - * Copyright (c) 2003-2018, Intel Corporation. All rights reserved.
> + * Copyright (c) 2003-2020, Intel Corporation. All rights reserved.
> * Intel Management Engine Interface (Intel MEI) Linux driver
> */
>
> @@ -476,7 +476,7 @@ static long mei_ioctl(struct file *file, unsigned int cmd, unsigned long data)
> case IOCTL_MEI_CONNECT_CLIENT:
> dev_dbg(dev->dev, ": IOCTL_MEI_CONNECT_CLIENT.\n");
> if (copy_from_user(&connect_data, (char __user *)data,
> - sizeof(struct mei_connect_client_data))) {
> + sizeof(connect_data))) {
> dev_dbg(dev->dev, "failed to copy data from userland\n");
> rets = -EFAULT;
> goto out;
> @@ -488,7 +488,7 @@ static long mei_ioctl(struct file *file, unsigned int cmd, unsigned long data)
>
> /* if all is ok, copying the data back to user. */
> if (copy_to_user((char __user *)data, &connect_data,
> - sizeof(struct mei_connect_client_data))) {
> + sizeof(connect_data))) {
> dev_dbg(dev->dev, "failed to copy data to userland\n");
> rets = -EFAULT;
> goto out;
>

2020-07-23 17:01:24

by Gustavo A. R. Silva

[permalink] [raw]

Subject: Re: [char-misc-next 1/6] mei: hbm: use sizeof of variable instead of struct type

On 7/23/20 10:13, Greg Kroah-Hartman wrote:
> On Thu, Jul 23, 2020 at 05:59:22PM +0300, Tomas Winkler wrote:
>> There is a possibility of bug when variable type has changed but
>> corresponding struct passed to the sizeof has not.
>>
>> Cc: Gustavo A. R. Silva <[email protected]>
>> Signed-off-by: Tomas Winkler <[email protected]>
>> ---
>> drivers/misc/mei/hbm.c | 74 ++++++++++++++++++------------------------
>> 1 file changed, 32 insertions(+), 42 deletions(-)
>
> This doesn't apply to my tree as I've applied Gustavo's patch. Should I
> revert that first?
>

Yep; this series doesn't take into account my patch. I'm OK with
reverting it, so we can apply this series.

Thanks
--
Gustavo

2020-07-23 17:30:03

by Greg Kroah-Hartman

[permalink] [raw]

Subject: Re: [char-misc-next 1/6] mei: hbm: use sizeof of variable instead of struct type

On Thu, Jul 23, 2020 at 12:04:06PM -0500, Gustavo A. R. Silva wrote:
>
>
> On 7/23/20 10:13, Greg Kroah-Hartman wrote:
> > On Thu, Jul 23, 2020 at 05:59:22PM +0300, Tomas Winkler wrote:
> >> There is a possibility of bug when variable type has changed but
> >> corresponding struct passed to the sizeof has not.
> >>
> >> Cc: Gustavo A. R. Silva <[email protected]>
> >> Signed-off-by: Tomas Winkler <[email protected]>
> >> ---
> >> drivers/misc/mei/hbm.c | 74 ++++++++++++++++++------------------------
> >> 1 file changed, 32 insertions(+), 42 deletions(-)
> >
> > This doesn't apply to my tree as I've applied Gustavo's patch. Should I
> > revert that first?
> >
>
> Yep; this series doesn't take into account my patch. I'm OK with
> reverting it, so we can apply this series.

Ok, will do, thanks!

greg k-h

2020-07-23 17:43:34

by Gustavo A. R. Silva

[permalink] [raw]

Subject: Re: [char-misc-next 1/6] mei: hbm: use sizeof of variable instead of struct type

On 7/23/20 09:59, Tomas Winkler wrote:
> There is a possibility of bug when variable type has changed but
> corresponding struct passed to the sizeof has not.
>
> Cc: Gustavo A. R. Silva <[email protected]>
> Signed-off-by: Tomas Winkler <[email protected]>
Reviewed-by: Gustavo A. R. Silva <[email protected]>

Thanks
--
Gustavo

> ---
> drivers/misc/mei/hbm.c | 74 ++++++++++++++++++------------------------
> 1 file changed, 32 insertions(+), 42 deletions(-)
>
> diff --git a/drivers/misc/mei/hbm.c b/drivers/misc/mei/hbm.c
> index a44094cdbc36..308caee86920 100644
> --- a/drivers/misc/mei/hbm.c
> +++ b/drivers/misc/mei/hbm.c
> @@ -1,6 +1,6 @@
> // SPDX-License-Identifier: GPL-2.0
> /*
> - * Copyright (c) 2003-2019, Intel Corporation. All rights reserved.
> + * Copyright (c) 2003-2020, Intel Corporation. All rights reserved.
> * Intel Management Engine Interface (Intel MEI) Linux driver
> */
> #include <linux/export.h>
> @@ -257,22 +257,21 @@ int mei_hbm_start_wait(struct mei_device *dev)
> int mei_hbm_start_req(struct mei_device *dev)
> {
> struct mei_msg_hdr mei_hdr;
> - struct hbm_host_version_request start_req;
> - const size_t len = sizeof(struct hbm_host_version_request);
> + struct hbm_host_version_request req;
> int ret;
>
> mei_hbm_reset(dev);
>
> - mei_hbm_hdr(&mei_hdr, len);
> + mei_hbm_hdr(&mei_hdr, sizeof(req));
>
> /* host start message */
> - memset(&start_req, 0, len);
> - start_req.hbm_cmd = HOST_START_REQ_CMD;
> - start_req.host_version.major_version = HBM_MAJOR_VERSION;
> - start_req.host_version.minor_version = HBM_MINOR_VERSION;
> + memset(&req, 0, sizeof(req));
> + req.hbm_cmd = HOST_START_REQ_CMD;
> + req.host_version.major_version = HBM_MAJOR_VERSION;
> + req.host_version.minor_version = HBM_MINOR_VERSION;
>
> dev->hbm_state = MEI_HBM_IDLE;
> - ret = mei_hbm_write_message(dev, &mei_hdr, &start_req);
> + ret = mei_hbm_write_message(dev, &mei_hdr, &req);
> if (ret) {
> dev_err(dev->dev, "version message write failed: ret = %d\n",
> ret);
> @@ -295,13 +294,12 @@ static int mei_hbm_dma_setup_req(struct mei_device *dev)
> {
> struct mei_msg_hdr mei_hdr;
> struct hbm_dma_setup_request req;
> - const size_t len = sizeof(struct hbm_dma_setup_request);
> unsigned int i;
> int ret;
>
> - mei_hbm_hdr(&mei_hdr, len);
> + mei_hbm_hdr(&mei_hdr, sizeof(req));
>
> - memset(&req, 0, len);
> + memset(&req, 0, sizeof(req));
> req.hbm_cmd = MEI_HBM_DMA_SETUP_REQ_CMD;
> for (i = 0; i < DMA_DSCR_NUM; i++) {
> phys_addr_t paddr;
> @@ -337,21 +335,19 @@ static int mei_hbm_dma_setup_req(struct mei_device *dev)
> static int mei_hbm_enum_clients_req(struct mei_device *dev)
> {
> struct mei_msg_hdr mei_hdr;
> - struct hbm_host_enum_request enum_req;
> - const size_t len = sizeof(struct hbm_host_enum_request);
> + struct hbm_host_enum_request req;
> int ret;
>
> /* enumerate clients */
> - mei_hbm_hdr(&mei_hdr, len);
> + mei_hbm_hdr(&mei_hdr, sizeof(req));
>
> - memset(&enum_req, 0, len);
> - enum_req.hbm_cmd = HOST_ENUM_REQ_CMD;
> - enum_req.flags |= dev->hbm_f_dc_supported ?
> - MEI_HBM_ENUM_F_ALLOW_ADD : 0;
> - enum_req.flags |= dev->hbm_f_ie_supported ?
> + memset(&req, 0, sizeof(req));
> + req.hbm_cmd = HOST_ENUM_REQ_CMD;
> + req.flags |= dev->hbm_f_dc_supported ? MEI_HBM_ENUM_F_ALLOW_ADD : 0;
> + req.flags |= dev->hbm_f_ie_supported ?
> MEI_HBM_ENUM_F_IMMEDIATE_ENUM : 0;
>
> - ret = mei_hbm_write_message(dev, &mei_hdr, &enum_req);
> + ret = mei_hbm_write_message(dev, &mei_hdr, &req);
> if (ret) {
> dev_err(dev->dev, "enumeration request write failed: ret = %d.\n",
> ret);
> @@ -380,7 +376,7 @@ static int mei_hbm_me_cl_add(struct mei_device *dev,
>
> mei_me_cl_rm_by_uuid(dev, uuid);
>
> - me_cl = kzalloc(sizeof(struct mei_me_client), GFP_KERNEL);
> + me_cl = kzalloc(sizeof(*me_cl), GFP_KERNEL);
> if (!me_cl)
> return -ENOMEM;
>
> @@ -408,14 +404,13 @@ 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);
> int ret;
>
> dev_dbg(dev->dev, "adding client response\n");
>
> - mei_hbm_hdr(&mei_hdr, len);
> + mei_hbm_hdr(&mei_hdr, sizeof(resp));
>
> - memset(&resp, 0, sizeof(struct hbm_add_client_response));
> + memset(&resp, 0, sizeof(resp));
> resp.hbm_cmd = MEI_HBM_ADD_CLIENT_RES_CMD;
> resp.me_addr = addr;
> resp.status = status;
> @@ -469,11 +464,10 @@ int mei_hbm_cl_notify_req(struct mei_device *dev,
>
> struct mei_msg_hdr mei_hdr;
> struct hbm_notification_request req;
> - const size_t len = sizeof(struct hbm_notification_request);
> int ret;
>
> - mei_hbm_hdr(&mei_hdr, len);
> - mei_hbm_cl_hdr(cl, MEI_HBM_NOTIFY_REQ_CMD, &req, len);
> + mei_hbm_hdr(&mei_hdr, sizeof(req));
> + mei_hbm_cl_hdr(cl, MEI_HBM_NOTIFY_REQ_CMD, &req, sizeof(req));
>
> req.start = start;
>
> @@ -580,8 +574,7 @@ static void mei_hbm_cl_notify(struct mei_device *dev,
> static int mei_hbm_prop_req(struct mei_device *dev, unsigned long start_idx)
> {
> struct mei_msg_hdr mei_hdr;
> - struct hbm_props_request prop_req;
> - const size_t len = sizeof(struct hbm_props_request);
> + struct hbm_props_request req;
> unsigned long addr;
> int ret;
>
> @@ -591,18 +584,17 @@ static int mei_hbm_prop_req(struct mei_device *dev, unsigned long start_idx)
> if (addr == MEI_CLIENTS_MAX) {
> dev->hbm_state = MEI_HBM_STARTED;
> mei_host_client_init(dev);
> -
> return 0;
> }
>
> - mei_hbm_hdr(&mei_hdr, len);
> + mei_hbm_hdr(&mei_hdr, sizeof(req));
>
> - memset(&prop_req, 0, sizeof(struct hbm_props_request));
> + memset(&req, 0, sizeof(req));
>
> - prop_req.hbm_cmd = HOST_CLIENT_PROPERTIES_REQ_CMD;
> - prop_req.me_addr = addr;
> + req.hbm_cmd = HOST_CLIENT_PROPERTIES_REQ_CMD;
> + req.me_addr = addr;
>
> - ret = mei_hbm_write_message(dev, &mei_hdr, &prop_req);
> + ret = mei_hbm_write_message(dev, &mei_hdr, &req);
> if (ret) {
> dev_err(dev->dev, "properties request write failed: ret = %d\n",
> ret);
> @@ -628,15 +620,14 @@ int mei_hbm_pg(struct mei_device *dev, u8 pg_cmd)
> {
> struct mei_msg_hdr mei_hdr;
> struct hbm_power_gate req;
> - const size_t len = sizeof(struct hbm_power_gate);
> int ret;
>
> if (!dev->hbm_f_pg_supported)
> return -EOPNOTSUPP;
>
> - mei_hbm_hdr(&mei_hdr, len);
> + mei_hbm_hdr(&mei_hdr, sizeof(req));
>
> - memset(&req, 0, len);
> + memset(&req, 0, sizeof(req));
> req.hbm_cmd = pg_cmd;
>
> ret = mei_hbm_write_message(dev, &mei_hdr, &req);
> @@ -657,11 +648,10 @@ static int mei_hbm_stop_req(struct mei_device *dev)
> {
> struct mei_msg_hdr mei_hdr;
> struct hbm_host_stop_request req;
> - const size_t len = sizeof(struct hbm_host_stop_request);
>
> - mei_hbm_hdr(&mei_hdr, len);
> + mei_hbm_hdr(&mei_hdr, sizeof(req));
>
> - memset(&req, 0, len);
> + memset(&req, 0, sizeof(req));
> req.hbm_cmd = HOST_STOP_REQ_CMD;
> req.reason = DRIVER_STOP_REQUEST;
>
>