[char-misc-next 0/8] mei security fixes and cleanups

[char-misc-next 0/8] mei security fixes and cleanups

[Tomas Winkler]

This series add fixes for possible security holes
mostly pointed out by static analysis tools and probably
not all of them are real with exception the nfc memory leak.

Alexander Usyskin (2):
  mei: replace stray pr_debug with dev_dbg
  mei: print correct device state during unexpected reset

Tomas Winkler (6):
  mei: debugfs: validate dev is not null
  mei: hbm: validate client index is not exceeding allocated array size
  mei: nfc: fix memory leak in error path
  mei: wd: host_init propagate error codes from called functions
  mei: bus: propagate error code returned by mei_me_cl_by_id
  mei: mei_cl_link remove duplicated check for open_handle_count

 drivers/misc/mei/bus.c     |  2 +-
 drivers/misc/mei/client.c  |  6 ------
 drivers/misc/mei/debugfs.c | 12 ++++++++++--
 drivers/misc/mei/hbm.c     |  6 ++++--
 drivers/misc/mei/init.c    |  8 ++++----
 drivers/misc/mei/nfc.c     | 10 ++++++----
 drivers/misc/mei/pci-me.c  |  2 +-
 drivers/misc/mei/wd.c      | 12 ++++++------
 8 files changed, 32 insertions(+), 26 deletions(-)

-- 
1.8.3.1

[char-misc-next 1/8] mei: debugfs: validate dev is not null

[Tomas Winkler]

make static analyzer happy and
validate dev argument before dereferencing

Signed-off-by: Tomas Winkler <tomas.winkler@intel.com>
---
 drivers/misc/mei/debugfs.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/drivers/misc/mei/debugfs.c b/drivers/misc/mei/debugfs.c
index e3870f2..9162491 100644
--- a/drivers/misc/mei/debugfs.c
+++ b/drivers/misc/mei/debugfs.c
@@ -30,11 +30,15 @@ static ssize_t mei_dbgfs_read_meclients(struct file *fp, char __user *ubuf,
 	struct mei_device *dev = fp->private_data;
 	struct mei_me_client *cl;
 	const size_t bufsz = 1024;
-	char *buf = kzalloc(bufsz, GFP_KERNEL);
+	char *buf;
 	int i;
 	int pos = 0;
 	int ret;
 
+	if (!dev)
+		return -ENODEV;
+
+	buf = kzalloc(bufsz, GFP_KERNEL);
 	if  (!buf)
 		return -ENOMEM;
 
@@ -80,10 +84,14 @@ static ssize_t mei_dbgfs_read_devstate(struct file *fp, char __user *ubuf,
 {
 	struct mei_device *dev = fp->private_data;
 	const size_t bufsz = 1024;
-	char *buf = kzalloc(bufsz, GFP_KERNEL);
+	char *buf;
 	int pos = 0;
 	int ret;
 
+	if (!dev)
+		return -ENODEV;
+
+	buf = kzalloc(bufsz, GFP_KERNEL);
 	if  (!buf)
 		return -ENOMEM;
 
-- 
1.8.3.1

Re: [char-misc-next 1/8] mei: debugfs: validate dev is not null

[Greg KH]

On Mon, Oct 21, 2013 at 10:05:36PM +0300, Tomas Winkler wrote:
> make static analyzer happy

That's a stupid analyzer.

Sorry, this case can never be true, so don't check for it.  Fix your
analyzer.

> and validate dev argument before dereferencing

Again, no need to do so, it's always set properly.

sorry, I can't take this.

greg k-h

RE: [char-misc-next 1/8] mei: debugfs: validate dev is not null

[Winkler, Tomas]

> 
> On Mon, Oct 21, 2013 at 10:05:36PM +0300, Tomas Winkler wrote:
> > make static analyzer happy
> 
> That's a stupid analyzer.
> 
> Sorry, this case can never be true, so don't check for it.  Fix your
> analyzer.
> 
> > and validate dev argument before dereferencing
> 
> Again, no need to do so, it's always set properly.
> 
> sorry, I can't take this.

Fair enough, I didn't like that patch myself now I have other confirmation that this is stupid.
 
Tomas

Re: [char-misc-next 1/8] mei: debugfs: validate dev is not null

[Greg KH]

On Wed, Oct 30, 2013 at 07:16:07AM +0000, Winkler, Tomas wrote:
> > 
> > On Mon, Oct 21, 2013 at 10:05:36PM +0300, Tomas Winkler wrote:
> > > make static analyzer happy
> > 
> > That's a stupid analyzer.
> > 
> > Sorry, this case can never be true, so don't check for it.  Fix your
> > analyzer.
> > 
> > > and validate dev argument before dereferencing
> > 
> > Again, no need to do so, it's always set properly.
> > 
> > sorry, I can't take this.
> 
> Fair enough, I didn't like that patch myself now I have other confirmation that this is stupid.

This is about the 10th time I have gotten patches that are somehow being
_forced_ by Intel developers to submit, and require me to say they are
stupid and wrong and push back on.

Someone needs to go stop that Intel code analyzer and push back on them
directly, don't rely on me being the one to do this, it only makes me
grumpy and mad when you all make me do your job on this :(

greg k-h

RE: [char-misc-next 1/8] mei: debugfs: validate dev is not null

[Winkler, Tomas]

> On Wed, Oct 30, 2013 at 07:16:07AM +0000, Winkler, Tomas wrote:
> > >
> > > On Mon, Oct 21, 2013 at 10:05:36PM +0300, Tomas Winkler wrote:
> > > > make static analyzer happy
> > >
> > > That's a stupid analyzer.
> > >
> > > Sorry, this case can never be true, so don't check for it.  Fix your
> > > analyzer.
> > >
> > > > and validate dev argument before dereferencing
> > >
> > > Again, no need to do so, it's always set properly.
> > >
> > > sorry, I can't take this.
> >
> > Fair enough, I didn't like that patch myself now I have other confirmation that
> this is stupid.
> 
> This is about the 10th time I have gotten patches that are somehow being
> _forced_ by Intel developers to submit, and require me to say they are
> stupid and wrong and push back on.
> 
> Someone needs to go stop that Intel code analyzer and push back on them
> directly, don't rely on me being the one to do this, it only makes me
> grumpy and mad when you all make me do your job on this :(
>
These tools and BKMs have a lot of false alarms, but sometimes they hit the nail.
It was just my personal bad call, to send this patch out.
But I really don't know about others, Intel is a big company. 

Thanks
Tomas

[char-misc-next 2/8] mei: hbm: validate client index is not exceeding allocated array size

[Tomas Winkler]

Signed-off-by: Tomas Winkler <tomas.winkler@intel.com>
---
 drivers/misc/mei/hbm.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/misc/mei/hbm.c b/drivers/misc/mei/hbm.c
index 9b3a0fb..0f5e8ca 100644
--- a/drivers/misc/mei/hbm.c
+++ b/drivers/misc/mei/hbm.c
@@ -228,8 +228,6 @@ static int mei_hbm_prop_req(struct mei_device *dev)
 	unsigned long client_num;
 
 
-	client_num = dev->me_client_presentation_num;
-
 	next_client_index = find_next_bit(dev->me_clients_map, MEI_CLIENTS_MAX,
 					  dev->me_client_index);
 
@@ -241,6 +239,10 @@ static int mei_hbm_prop_req(struct mei_device *dev)
 		return 0;
 	}
 
+	client_num = dev->me_client_presentation_num;
+	if (WARN_ON(dev->me_clients_num <= client_num))
+		return -EIO;
+
 	dev->me_clients[client_num].client_id = next_client_index;
 	dev->me_clients[client_num].mei_flow_ctrl_creds = 0;
 
-- 
1.8.3.1

Re: [char-misc-next 2/8] mei: hbm: validate client index is not exceeding allocated array size

[Greg KH]

On Mon, Oct 21, 2013 at 10:05:37PM +0300, Tomas Winkler wrote:
> Signed-off-by: Tomas Winkler <tomas.winkler@intel.com>
> ---
>  drivers/misc/mei/hbm.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/misc/mei/hbm.c b/drivers/misc/mei/hbm.c
> index 9b3a0fb..0f5e8ca 100644
> --- a/drivers/misc/mei/hbm.c
> +++ b/drivers/misc/mei/hbm.c
> @@ -228,8 +228,6 @@ static int mei_hbm_prop_req(struct mei_device *dev)
>  	unsigned long client_num;
>  
>  
> -	client_num = dev->me_client_presentation_num;
> -
>  	next_client_index = find_next_bit(dev->me_clients_map, MEI_CLIENTS_MAX,
>  					  dev->me_client_index);
>  
> @@ -241,6 +239,10 @@ static int mei_hbm_prop_req(struct mei_device *dev)
>  		return 0;
>  	}
>  
> +	client_num = dev->me_client_presentation_num;
> +	if (WARN_ON(dev->me_clients_num <= client_num))
> +		return -EIO;

How can this happen?  Why is spitting out a huge warning in the syslog
going to help anything?  If a user can do this, then great, now you can
DoS your syslog :(

If a user can't do this, then why tell them, it's your driver's bug that
you should just fix.

greg k-h

RE: [char-misc-next 2/8] mei: hbm: validate client index is not exceeding allocated array size

[Winkler, Tomas]

> > ---
> >  drivers/misc/mei/hbm.c | 6 ++++--
> >  1 file changed, 4 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/misc/mei/hbm.c b/drivers/misc/mei/hbm.c
> > index 9b3a0fb..0f5e8ca 100644
> > --- a/drivers/misc/mei/hbm.c
> > +++ b/drivers/misc/mei/hbm.c
> > @@ -228,8 +228,6 @@ static int mei_hbm_prop_req(struct mei_device *dev)
> >  	unsigned long client_num;
> >
> >
> > -	client_num = dev->me_client_presentation_num;
> > -
> >  	next_client_index = find_next_bit(dev->me_clients_map,
> MEI_CLIENTS_MAX,
> >  					  dev->me_client_index);
> >
> > @@ -241,6 +239,10 @@ static int mei_hbm_prop_req(struct mei_device *dev)
> >  		return 0;
> >  	}
> >
> > +	client_num = dev->me_client_presentation_num;
> > +	if (WARN_ON(dev->me_clients_num <= client_num))
> > +		return -EIO;
> 
> How can this happen?  Why is spitting out a huge warning in the syslog
> going to help anything?  If a user can do this, then great, now you can
> DoS your syslog :(
> 
> If a user can't do this, then why tell them, it's your driver's bug that
> you should just fix.

This somehow should guard buffer overflow allocated of size dev->me_clients_num
In theory this can happen only if something go wrong in hardware initialization or there is some other security hole that can change client_num.

After this happen you probably won't be able to use the driver anyhow so I do not expect DoS on the syslog, but we can drop the WARN_ON
but I would stick with the check

Thanks
Tomas

Re: [char-misc-next 2/8] mei: hbm: validate client index is not exceeding allocated array size

[Greg KH]

On Wed, Oct 30, 2013 at 07:31:06AM +0000, Winkler, Tomas wrote:
> 
> 
> > > ---
> > >  drivers/misc/mei/hbm.c | 6 ++++--
> > >  1 file changed, 4 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/misc/mei/hbm.c b/drivers/misc/mei/hbm.c
> > > index 9b3a0fb..0f5e8ca 100644
> > > --- a/drivers/misc/mei/hbm.c
> > > +++ b/drivers/misc/mei/hbm.c
> > > @@ -228,8 +228,6 @@ static int mei_hbm_prop_req(struct mei_device *dev)
> > >  	unsigned long client_num;
> > >
> > >
> > > -	client_num = dev->me_client_presentation_num;
> > > -
> > >  	next_client_index = find_next_bit(dev->me_clients_map,
> > MEI_CLIENTS_MAX,
> > >  					  dev->me_client_index);
> > >
> > > @@ -241,6 +239,10 @@ static int mei_hbm_prop_req(struct mei_device *dev)
> > >  		return 0;
> > >  	}
> > >
> > > +	client_num = dev->me_client_presentation_num;
> > > +	if (WARN_ON(dev->me_clients_num <= client_num))
> > > +		return -EIO;
> > 
> > How can this happen?  Why is spitting out a huge warning in the syslog
> > going to help anything?  If a user can do this, then great, now you can
> > DoS your syslog :(
> > 
> > If a user can't do this, then why tell them, it's your driver's bug that
> > you should just fix.
> 
> This somehow should guard buffer overflow allocated of size dev->me_clients_num
> In theory this can happen only if something go wrong in hardware
> initialization or there is some other security hole that can change
> client_num.

What _kind_ of "security hole" could ever change that number?  Where
does it come from?  Who can modify it?  If you don't know that now then
we have worse problems...

greg k-h

RE: [char-misc-next 2/8] mei: hbm: validate client index is not exceeding allocated array size

[Winkler, Tomas]

> >
> > This somehow should guard buffer overflow allocated of size dev-
> >me_clients_num
> > In theory this can happen only if something go wrong in hardware
> > initialization or there is some other security hole that can change
> > client_num.
> 
> What _kind_ of "security hole" could ever change that number?  Where
> does it come from?  Who can modify it?  If you don't know that now then
> we have worse problems...

The allocation of me_clients  arrays of mei_clients_num is happening on ME enumeration message,
While  the filling out the array is looping over get properties message which is bounded by MEI_CLIENTS_MAX,
so the  overflow is indeed possible, of course only on some faulty HW.  We had such errors only on new 
HW bring ups. 

Thanks
Tomas

[char-misc-next 3/8] mei: nfc: fix memory leak in error path

[Tomas Winkler]

The flow may reach the err label without freeing cl and cl_info

cl and cl_info weren't assigned to ndev->cl and cl_info
so they weren't freed in mei_nfc_free called on error path

Cc: <stable@vger.kernel.org> # 3.10+
Cc: Samuel Ortiz <sameo@linux.intel.com>
Signed-off-by: Tomas Winkler <tomas.winkler@intel.com>
---
 drivers/misc/mei/nfc.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/misc/mei/nfc.c b/drivers/misc/mei/nfc.c
index d0c6907..994ca4a 100644
--- a/drivers/misc/mei/nfc.c
+++ b/drivers/misc/mei/nfc.c
@@ -485,8 +485,11 @@ int mei_nfc_host_init(struct mei_device *dev)
 	if (ndev->cl_info)
 		return 0;
 
-	cl_info = mei_cl_allocate(dev);
-	cl = mei_cl_allocate(dev);
+	ndev->cl_info = mei_cl_allocate(dev);
+	ndev->cl = mei_cl_allocate(dev);
+
+	cl = ndev->cl;
+	cl_info = ndev->cl_info;
 
 	if (!cl || !cl_info) {
 		ret = -ENOMEM;
@@ -527,10 +530,9 @@ int mei_nfc_host_init(struct mei_device *dev)
 
 	cl->device_uuid = mei_nfc_guid;
 
+
 	list_add_tail(&cl->device_link, &dev->device_list);
 
-	ndev->cl_info = cl_info;
-	ndev->cl = cl;
 	ndev->req_id = 1;
 
 	INIT_WORK(&ndev->init_work, mei_nfc_init);
-- 
1.8.3.1

[char-misc-next 4/8] mei: wd: host_init propagate error codes from called functions

[Tomas Winkler]

Propagate error codes from called functions, they are correct.

Signed-off-by: Tomas Winkler <tomas.winkler@intel.com>
---
 drivers/misc/mei/wd.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/misc/mei/wd.c b/drivers/misc/mei/wd.c
index b892143..9e35421 100644
--- a/drivers/misc/mei/wd.c
+++ b/drivers/misc/mei/wd.c
@@ -60,7 +60,7 @@ static void mei_wd_set_start_timeout(struct mei_device *dev, u16 timeout)
 int mei_wd_host_init(struct mei_device *dev)
 {
 	struct mei_cl *cl = &dev->wd_cl;
-	int i;
+	int id;
 	int ret;
 
 	mei_cl_init(cl, dev);
@@ -70,19 +70,19 @@ int mei_wd_host_init(struct mei_device *dev)
 
 
 	/* check for valid client id */
-	i = mei_me_cl_by_uuid(dev, &mei_wd_guid);
-	if (i < 0) {
+	id = mei_me_cl_by_uuid(dev, &mei_wd_guid);
+	if (id < 0) {
 		dev_info(&dev->pdev->dev, "wd: failed to find the client\n");
-		return -ENOENT;
+		return id;
 	}
 
-	cl->me_client_id = dev->me_clients[i].client_id;
+	cl->me_client_id = dev->me_clients[id].client_id;
 
 	ret = mei_cl_link(cl, MEI_WD_HOST_CLIENT_ID);
 
 	if (ret < 0) {
 		dev_info(&dev->pdev->dev, "wd: failed link client\n");
-		return -ENOENT;
+		return ret;
 	}
 
 	cl->state = MEI_FILE_CONNECTING;
-- 
1.8.3.1

[char-misc-next 5/8] mei: bus: propagate error code returned by mei_me_cl_by_id

[Tomas Winkler]

no need to change error code value returned by
mei_me_cl_by_id, just propagate it on

Signed-off-by: Tomas Winkler <tomas.winkler@intel.com>
---
 drivers/misc/mei/bus.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/misc/mei/bus.c b/drivers/misc/mei/bus.c
index cd2033c..4bc7d62 100644
--- a/drivers/misc/mei/bus.c
+++ b/drivers/misc/mei/bus.c
@@ -245,7 +245,7 @@ static int ___mei_cl_send(struct mei_cl *cl, u8 *buf, size_t length,
 	/* Check if we have an ME client device */
 	id = mei_me_cl_by_id(dev, cl->me_client_id);
 	if (id < 0)
-		return -ENODEV;
+		return id;
 
 	if (length > dev->me_clients[id].props.max_msg_length)
 		return -EINVAL;
-- 
1.8.3.1

[char-misc-next 6/8] mei: mei_cl_link remove duplicated check for open_handle_count

[Tomas Winkler]

Signed-off-by: Tomas Winkler <tomas.winkler@intel.com>
---
 drivers/misc/mei/client.c | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/drivers/misc/mei/client.c b/drivers/misc/mei/client.c
index 0ccc22c..87c96e4 100644
--- a/drivers/misc/mei/client.c
+++ b/drivers/misc/mei/client.c
@@ -299,12 +299,6 @@ int mei_cl_link(struct mei_cl *cl, int id)
 		return -EMFILE;
 	}
 
-	if (dev->open_handle_count >= MEI_MAX_OPEN_HANDLE_COUNT) {
-		dev_err(&dev->pdev->dev, "open_handle_count exceded %d",
-			MEI_MAX_OPEN_HANDLE_COUNT);
-		return -ENOENT;
-	}
-
 	dev->open_handle_count++;
 
 	cl->host_client_id = id;
-- 
1.8.3.1

[char-misc-next 7/8] mei: replace stray pr_debug with dev_dbg

[Tomas Winkler]

From: Alexander Usyskin <alexander.usyskin@intel.com>

Driver better use dev_dbg, not pr_debug.

Signed-off-by: Alexander Usyskin <alexander.usyskin@intel.com>
Signed-off-by: Tomas Winkler <tomas.winkler@intel.com>
---
 drivers/misc/mei/pci-me.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/misc/mei/pci-me.c b/drivers/misc/mei/pci-me.c
index 1bf300e..b96205a 100644
--- a/drivers/misc/mei/pci-me.c
+++ b/drivers/misc/mei/pci-me.c
@@ -190,7 +190,7 @@ static int mei_me_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 
 	schedule_delayed_work(&dev->timer_work, HZ);
 
-	pr_debug("initialization successful.\n");
+	dev_dbg(&pdev->dev, "initialization successful.\n");
 
 	return 0;
 
-- 
1.8.3.1

[char-misc-next 8/8] mei: print correct device state during unexpected reset

[Tomas Winkler]

From: Alexander Usyskin <alexander.usyskin@intel.com>

Move the unexpected state print to the beginning of mei_reset,
thus printing right state.

Signed-off-by: Alexander Usyskin <alexander.usyskin@intel.com>
Signed-off-by: Tomas Winkler <tomas.winkler@intel.com>
---
 drivers/misc/mei/init.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/misc/mei/init.c b/drivers/misc/mei/init.c
index 4c93e3d..7dfc7a4 100644
--- a/drivers/misc/mei/init.c
+++ b/drivers/misc/mei/init.c
@@ -147,6 +147,10 @@ void mei_reset(struct mei_device *dev, int interrupts_enabled)
 			dev->dev_state != MEI_DEV_POWER_DOWN &&
 			dev->dev_state != MEI_DEV_POWER_UP);
 
+	if (unexpected)
+		dev_warn(&dev->pdev->dev, "unexpected reset: dev_state = %s\n",
+			 mei_dev_state_str(dev->dev_state));
+
 	ret = mei_hw_reset(dev, interrupts_enabled);
 	if (ret) {
 		dev_err(&dev->pdev->dev, "hw reset failed disabling the device\n");
@@ -186,10 +190,6 @@ void mei_reset(struct mei_device *dev, int interrupts_enabled)
 	dev->rd_msg_hdr = 0;
 	dev->wd_pending = false;
 
-	if (unexpected)
-		dev_warn(&dev->pdev->dev, "unexpected reset: dev_state = %s\n",
-			 mei_dev_state_str(dev->dev_state));
-
 	if (!interrupts_enabled) {
 		dev_dbg(&dev->pdev->dev, "intr not enabled end of reset\n");
 		return;
-- 
1.8.3.1