[PATCHv3 0/4] OPAL patches

[PATCHv3 0/4] OPAL patches

[Jon Derrick]

Just a couple of fixes for sed-opal to prevent faulty firmware from
allowing us to go off in the weeds, and a helper to remove some
duplicate code.

v2->v3:
corrected the bad calculation on the response parser check and changed
it to only check the subpacket length

v1->v2:
left tok->len as a size_t
got everyone important on the same email thread


Jon Derrick (4):
  block/sed: Use ssize_t on atom parsers to return errors
  block/sed: Add helper to qualify response tokens
  block/sed: Check received header lengths
  MAINTAINERS: Remove powerpc's opal match

 MAINTAINERS      |   1 -
 block/sed-opal.c | 124 +++++++++++++++++++++++++++----------------------------
 2 files changed, 60 insertions(+), 65 deletions(-)

-- 
1.8.3.1

[PATCHv3 0/4] OPAL patches

[Jon Derrick]

Just a couple of fixes for sed-opal to prevent faulty firmware from
allowing us to go off in the weeds, and a helper to remove some
duplicate code.

v2->v3:
corrected the bad calculation on the response parser check and changed
it to only check the subpacket length

v1->v2:
left tok->len as a size_t
got everyone important on the same email thread


Jon Derrick (4):
  block/sed: Use ssize_t on atom parsers to return errors
  block/sed: Add helper to qualify response tokens
  block/sed: Check received header lengths
  MAINTAINERS: Remove powerpc's opal match

 MAINTAINERS      |   1 -
 block/sed-opal.c | 124 +++++++++++++++++++++++++++----------------------------
 2 files changed, 60 insertions(+), 65 deletions(-)

-- 
1.8.3.1

[PATCHv3 1/4] block/sed: Use ssize_t on atom parsers to return errors

[Jon Derrick]

The short atom parser can return an errno from decoding but does not
currently return the error as a signed value. Convert all of the parsers
to ssize_t.

Signed-off-by: Jon Derrick <jonathan.derrick@intel.com>
---
 block/sed-opal.c | 28 ++++++++++++++--------------
 1 file changed, 14 insertions(+), 14 deletions(-)

diff --git a/block/sed-opal.c b/block/sed-opal.c
index e95b8a5..77623ad 100644
--- a/block/sed-opal.c
+++ b/block/sed-opal.c
@@ -635,8 +635,8 @@ static enum opal_token response_get_token(const struct parsed_resp *resp,
 	return tok->pos[0];
 }
 
-static size_t response_parse_tiny(struct opal_resp_tok *tok,
-				  const u8 *pos)
+static ssize_t response_parse_tiny(struct opal_resp_tok *tok,
+				   const u8 *pos)
 {
 	tok->pos = pos;
 	tok->len = 1;
@@ -652,8 +652,8 @@ static size_t response_parse_tiny(struct opal_resp_tok *tok,
 	return tok->len;
 }
 
-static size_t response_parse_short(struct opal_resp_tok *tok,
-				   const u8 *pos)
+static ssize_t response_parse_short(struct opal_resp_tok *tok,
+				    const u8 *pos)
 {
 	tok->pos = pos;
 	tok->len = (pos[0] & SHORT_ATOM_LEN_MASK) + 1;
@@ -665,7 +665,7 @@ static size_t response_parse_short(struct opal_resp_tok *tok,
 		tok->type = OPAL_DTA_TOKENID_SINT;
 	} else {
 		u64 u_integer = 0;
-		int i, b = 0;
+		ssize_t i, b = 0;
 
 		tok->type = OPAL_DTA_TOKENID_UINT;
 		if (tok->len > 9) {
@@ -682,8 +682,8 @@ static size_t response_parse_short(struct opal_resp_tok *tok,
 	return tok->len;
 }
 
-static size_t response_parse_medium(struct opal_resp_tok *tok,
-				    const u8 *pos)
+static ssize_t response_parse_medium(struct opal_resp_tok *tok,
+				     const u8 *pos)
 {
 	tok->pos = pos;
 	tok->len = (((pos[0] & MEDIUM_ATOM_LEN_MASK) << 8) | pos[1]) + 2;
@@ -699,8 +699,8 @@ static size_t response_parse_medium(struct opal_resp_tok *tok,
 	return tok->len;
 }
 
-static size_t response_parse_long(struct opal_resp_tok *tok,
-				  const u8 *pos)
+static ssize_t response_parse_long(struct opal_resp_tok *tok,
+				   const u8 *pos)
 {
 	tok->pos = pos;
 	tok->len = ((pos[1] << 16) | (pos[2] << 8) | pos[3]) + 4;
@@ -716,8 +716,8 @@ static size_t response_parse_long(struct opal_resp_tok *tok,
 	return tok->len;
 }
 
-static size_t response_parse_token(struct opal_resp_tok *tok,
-				   const u8 *pos)
+static ssize_t response_parse_token(struct opal_resp_tok *tok,
+				    const u8 *pos)
 {
 	tok->pos = pos;
 	tok->len = 1;
@@ -734,7 +734,7 @@ static int response_parse(const u8 *buf, size_t length,
 	struct opal_resp_tok *iter;
 	int num_entries = 0;
 	int total;
-	size_t token_length;
+	ssize_t token_length;
 	const u8 *pos;
 
 	if (!buf)
@@ -780,8 +780,8 @@ static int response_parse(const u8 *buf, size_t length,
 		else /* TOKEN */
 			token_length = response_parse_token(iter, pos);
 
-		if (token_length == -EINVAL)
-			return -EINVAL;
+		if (token_length < 0)
+			return token_length;
 
 		pos += token_length;
 		total -= token_length;
-- 
1.8.3.1

[PATCHv3 1/4] block/sed: Use ssize_t on atom parsers to return errors

[Jon Derrick]

The short atom parser can return an errno from decoding but does not
currently return the error as a signed value. Convert all of the parsers
to ssize_t.

Signed-off-by: Jon Derrick <jonathan.derrick@intel.com>
---
 block/sed-opal.c | 28 ++++++++++++++--------------
 1 file changed, 14 insertions(+), 14 deletions(-)

diff --git a/block/sed-opal.c b/block/sed-opal.c
index e95b8a5..77623ad 100644
--- a/block/sed-opal.c
+++ b/block/sed-opal.c
@@ -635,8 +635,8 @@ static enum opal_token response_get_token(const struct parsed_resp *resp,
 	return tok->pos[0];
 }
 
-static size_t response_parse_tiny(struct opal_resp_tok *tok,
-				  const u8 *pos)
+static ssize_t response_parse_tiny(struct opal_resp_tok *tok,
+				   const u8 *pos)
 {
 	tok->pos = pos;
 	tok->len = 1;
@@ -652,8 +652,8 @@ static size_t response_parse_tiny(struct opal_resp_tok *tok,
 	return tok->len;
 }
 
-static size_t response_parse_short(struct opal_resp_tok *tok,
-				   const u8 *pos)
+static ssize_t response_parse_short(struct opal_resp_tok *tok,
+				    const u8 *pos)
 {
 	tok->pos = pos;
 	tok->len = (pos[0] & SHORT_ATOM_LEN_MASK) + 1;
@@ -665,7 +665,7 @@ static size_t response_parse_short(struct opal_resp_tok *tok,
 		tok->type = OPAL_DTA_TOKENID_SINT;
 	} else {
 		u64 u_integer = 0;
-		int i, b = 0;
+		ssize_t i, b = 0;
 
 		tok->type = OPAL_DTA_TOKENID_UINT;
 		if (tok->len > 9) {
@@ -682,8 +682,8 @@ static size_t response_parse_short(struct opal_resp_tok *tok,
 	return tok->len;
 }
 
-static size_t response_parse_medium(struct opal_resp_tok *tok,
-				    const u8 *pos)
+static ssize_t response_parse_medium(struct opal_resp_tok *tok,
+				     const u8 *pos)
 {
 	tok->pos = pos;
 	tok->len = (((pos[0] & MEDIUM_ATOM_LEN_MASK) << 8) | pos[1]) + 2;
@@ -699,8 +699,8 @@ static size_t response_parse_medium(struct opal_resp_tok *tok,
 	return tok->len;
 }
 
-static size_t response_parse_long(struct opal_resp_tok *tok,
-				  const u8 *pos)
+static ssize_t response_parse_long(struct opal_resp_tok *tok,
+				   const u8 *pos)
 {
 	tok->pos = pos;
 	tok->len = ((pos[1] << 16) | (pos[2] << 8) | pos[3]) + 4;
@@ -716,8 +716,8 @@ static size_t response_parse_long(struct opal_resp_tok *tok,
 	return tok->len;
 }
 
-static size_t response_parse_token(struct opal_resp_tok *tok,
-				   const u8 *pos)
+static ssize_t response_parse_token(struct opal_resp_tok *tok,
+				    const u8 *pos)
 {
 	tok->pos = pos;
 	tok->len = 1;
@@ -734,7 +734,7 @@ static int response_parse(const u8 *buf, size_t length,
 	struct opal_resp_tok *iter;
 	int num_entries = 0;
 	int total;
-	size_t token_length;
+	ssize_t token_length;
 	const u8 *pos;
 
 	if (!buf)
@@ -780,8 +780,8 @@ static int response_parse(const u8 *buf, size_t length,
 		else /* TOKEN */
 			token_length = response_parse_token(iter, pos);
 
-		if (token_length == -EINVAL)
-			return -EINVAL;
+		if (token_length < 0)
+			return token_length;
 
 		pos += token_length;
 		total -= token_length;
-- 
1.8.3.1

Re: [PATCHv3 1/4] block/sed: Use ssize_t on atom parsers to return errors

[Scott Bauer]

On Wed, Feb 15, 2017 at 12:42:06PM -0700, Jon Derrick wrote:
> The short atom parser can return an errno from decoding but does not
> currently return the error as a signed value. Convert all of the parsers
> to ssize_t.
> 
> Signed-off-by: Jon Derrick <jonathan.derrick@intel.com>
Reviewed-by: Scott Bauer <scott.bauer@intel.com>

[PATCHv3 2/4] block/sed: Add helper to qualify response tokens

[Jon Derrick]

Add helper which verifies the response token is valid and matches the
expected value. Merges token_type and response_get_token.

Signed-off-by: Jon Derrick <jonathan.derrick@intel.com>
---
 block/sed-opal.c | 61 +++++++++++++++++++++++---------------------------------
 1 file changed, 25 insertions(+), 36 deletions(-)

diff --git a/block/sed-opal.c b/block/sed-opal.c
index 77623ad..d6dd604 100644
--- a/block/sed-opal.c
+++ b/block/sed-opal.c
@@ -591,48 +591,25 @@ static int cmd_finalize(struct opal_dev *cmd, u32 hsn, u32 tsn)
 	return 0;
 }
 
-static enum opal_response_token token_type(const struct parsed_resp *resp,
-					   int n)
+static const struct opal_resp_tok *response_get_token(
+				const struct parsed_resp *resp,
+				int n)
 {
 	const struct opal_resp_tok *tok;
 
 	if (n >= resp->num) {
 		pr_err("Token number doesn't exist: %d, resp: %d\n",
 		       n, resp->num);
-		return OPAL_DTA_TOKENID_INVALID;
+		return ERR_PTR(-EINVAL);
 	}
 
 	tok = &resp->toks[n];
 	if (tok->len == 0) {
 		pr_err("Token length must be non-zero\n");
-		return OPAL_DTA_TOKENID_INVALID;
+		return ERR_PTR(-EINVAL);
 	}
 
-	return tok->type;
-}
-
-/*
- * This function returns 0 in case of invalid token. One should call
- * token_type() first to find out if the token is valid or not.
- */
-static enum opal_token response_get_token(const struct parsed_resp *resp,
-					  int n)
-{
-	const struct opal_resp_tok *tok;
-
-	if (n >= resp->num) {
-		pr_err("Token number doesn't exist: %d, resp: %d\n",
-		       n, resp->num);
-		return 0;
-	}
-
-	tok = &resp->toks[n];
-	if (tok->len == 0) {
-		pr_err("Token length must be non-zero\n");
-		return 0;
-	}
-
-	return tok->pos[0];
+	return tok;
 }
 
 static ssize_t response_parse_tiny(struct opal_resp_tok *tok,
@@ -851,20 +828,32 @@ static u64 response_get_u64(const struct parsed_resp *resp, int n)
 	return resp->toks[n].stored.u;
 }
 
+static bool response_token_matches(const struct opal_resp_tok *token, u8 match)
+{
+	if (IS_ERR_OR_NULL(token) ||
+	    token->type != OPAL_DTA_TOKENID_TOKEN ||
+	    token->pos[0] != match)
+		return false;
+	return true;
+}
+
 static u8 response_status(const struct parsed_resp *resp)
 {
-	if (token_type(resp, 0) == OPAL_DTA_TOKENID_TOKEN &&
-	    response_get_token(resp, 0) == OPAL_ENDOFSESSION) {
+	const struct opal_resp_tok *tok;
+
+	tok = response_get_token(resp, 0);
+	if (response_token_matches(tok, OPAL_ENDOFSESSION))
 		return 0;
-	}
 
 	if (resp->num < 5)
 		return DTAERROR_NO_METHOD_STATUS;
 
-	if (token_type(resp, resp->num - 1) != OPAL_DTA_TOKENID_TOKEN ||
-	    token_type(resp, resp->num - 5) != OPAL_DTA_TOKENID_TOKEN ||
-	    response_get_token(resp, resp->num - 1) != OPAL_ENDLIST ||
-	    response_get_token(resp, resp->num - 5) != OPAL_STARTLIST)
+	tok = response_get_token(resp, resp->num - 5);
+	if (!response_token_matches(tok, OPAL_STARTLIST))
+		return DTAERROR_NO_METHOD_STATUS;
+
+	tok = response_get_token(resp, resp->num - 1);
+	if (!response_token_matches(tok, OPAL_ENDLIST))
 		return DTAERROR_NO_METHOD_STATUS;
 
 	return response_get_u64(resp, resp->num - 4);
-- 
1.8.3.1

[PATCHv3 2/4] block/sed: Add helper to qualify response tokens

[Jon Derrick]

Add helper which verifies the response token is valid and matches the
expected value. Merges token_type and response_get_token.

Signed-off-by: Jon Derrick <jonathan.derrick@intel.com>
---
 block/sed-opal.c | 61 +++++++++++++++++++++++---------------------------------
 1 file changed, 25 insertions(+), 36 deletions(-)

diff --git a/block/sed-opal.c b/block/sed-opal.c
index 77623ad..d6dd604 100644
--- a/block/sed-opal.c
+++ b/block/sed-opal.c
@@ -591,48 +591,25 @@ static int cmd_finalize(struct opal_dev *cmd, u32 hsn, u32 tsn)
 	return 0;
 }
 
-static enum opal_response_token token_type(const struct parsed_resp *resp,
-					   int n)
+static const struct opal_resp_tok *response_get_token(
+				const struct parsed_resp *resp,
+				int n)
 {
 	const struct opal_resp_tok *tok;
 
 	if (n >= resp->num) {
 		pr_err("Token number doesn't exist: %d, resp: %d\n",
 		       n, resp->num);
-		return OPAL_DTA_TOKENID_INVALID;
+		return ERR_PTR(-EINVAL);
 	}
 
 	tok = &resp->toks[n];
 	if (tok->len == 0) {
 		pr_err("Token length must be non-zero\n");
-		return OPAL_DTA_TOKENID_INVALID;
+		return ERR_PTR(-EINVAL);
 	}
 
-	return tok->type;
-}
-
-/*
- * This function returns 0 in case of invalid token. One should call
- * token_type() first to find out if the token is valid or not.
- */
-static enum opal_token response_get_token(const struct parsed_resp *resp,
-					  int n)
-{
-	const struct opal_resp_tok *tok;
-
-	if (n >= resp->num) {
-		pr_err("Token number doesn't exist: %d, resp: %d\n",
-		       n, resp->num);
-		return 0;
-	}
-
-	tok = &resp->toks[n];
-	if (tok->len == 0) {
-		pr_err("Token length must be non-zero\n");
-		return 0;
-	}
-
-	return tok->pos[0];
+	return tok;
 }
 
 static ssize_t response_parse_tiny(struct opal_resp_tok *tok,
@@ -851,20 +828,32 @@ static u64 response_get_u64(const struct parsed_resp *resp, int n)
 	return resp->toks[n].stored.u;
 }
 
+static bool response_token_matches(const struct opal_resp_tok *token, u8 match)
+{
+	if (IS_ERR_OR_NULL(token) ||
+	    token->type != OPAL_DTA_TOKENID_TOKEN ||
+	    token->pos[0] != match)
+		return false;
+	return true;
+}
+
 static u8 response_status(const struct parsed_resp *resp)
 {
-	if (token_type(resp, 0) == OPAL_DTA_TOKENID_TOKEN &&
-	    response_get_token(resp, 0) == OPAL_ENDOFSESSION) {
+	const struct opal_resp_tok *tok;
+
+	tok = response_get_token(resp, 0);
+	if (response_token_matches(tok, OPAL_ENDOFSESSION))
 		return 0;
-	}
 
 	if (resp->num < 5)
 		return DTAERROR_NO_METHOD_STATUS;
 
-	if (token_type(resp, resp->num - 1) != OPAL_DTA_TOKENID_TOKEN ||
-	    token_type(resp, resp->num - 5) != OPAL_DTA_TOKENID_TOKEN ||
-	    response_get_token(resp, resp->num - 1) != OPAL_ENDLIST ||
-	    response_get_token(resp, resp->num - 5) != OPAL_STARTLIST)
+	tok = response_get_token(resp, resp->num - 5);
+	if (!response_token_matches(tok, OPAL_STARTLIST))
+		return DTAERROR_NO_METHOD_STATUS;
+
+	tok = response_get_token(resp, resp->num - 1);
+	if (!response_token_matches(tok, OPAL_ENDLIST))
 		return DTAERROR_NO_METHOD_STATUS;
 
 	return response_get_u64(resp, resp->num - 4);
-- 
1.8.3.1

Re: [PATCHv3 2/4] block/sed: Add helper to qualify response tokens

[Scott Bauer]

On Wed, Feb 15, 2017 at 12:42:07PM -0700, Jon Derrick wrote:
> Add helper which verifies the response token is valid and matches the
> expected value. Merges token_type and response_get_token.
> 
> Signed-off-by: Jon Derrick <jonathan.derrick@intel.com>
> ---
>  block/sed-opal.c | 61 +++++++++++++++++++++++---------------------------------
>  1 file changed, 25 insertions(+), 36 deletions(-)
> 
> diff --git a/block/sed-opal.c b/block/sed-opal.c
> index 77623ad..d6dd604 100644
> --- a/block/sed-opal.c
> +++ b/block/sed-opal.c
> @@ -591,48 +591,25 @@ static int cmd_finalize(struct opal_dev *cmd, u32 hsn, u32 tsn)
>  	return 0;
>  }
>  
> -static enum opal_response_token token_type(const struct parsed_resp *resp,
> -					   int n)
> +static const struct opal_resp_tok *response_get_token(
> +				const struct parsed_resp *resp,
> +				int n)
>  {
>  	const struct opal_resp_tok *tok;
>  
>  	if (n >= resp->num) {
>  		pr_err("Token number doesn't exist: %d, resp: %d\n",
>  		       n, resp->num);
> -		return OPAL_DTA_TOKENID_INVALID;
> +		return ERR_PTR(-EINVAL);
>  	}
>  
>  	tok = &resp->toks[n];
>  	if (tok->len == 0) {
>  		pr_err("Token length must be non-zero\n");
> -		return OPAL_DTA_TOKENID_INVALID;
> +		return ERR_PTR(-EINVAL);
>  	}
>  
> -	return tok->type;
> -}
> -
> -/*
> - * This function returns 0 in case of invalid token. One should call
> - * token_type() first to find out if the token is valid or not.
> - */
> -static enum opal_token response_get_token(const struct parsed_resp *resp,
> -					  int n)
> -{
> -	const struct opal_resp_tok *tok;
> -
> -	if (n >= resp->num) {
> -		pr_err("Token number doesn't exist: %d, resp: %d\n",
> -		       n, resp->num);
> -		return 0;
> -	}
> -
> -	tok = &resp->toks[n];
> -	if (tok->len == 0) {
> -		pr_err("Token length must be non-zero\n");
> -		return 0;
> -	}
> -
> -	return tok->pos[0];
> +	return tok;
>  }
>  
>  static ssize_t response_parse_tiny(struct opal_resp_tok *tok,
> @@ -851,20 +828,32 @@ static u64 response_get_u64(const struct parsed_resp *resp, int n)
>  	return resp->toks[n].stored.u;
>  }
>  
> +static bool response_token_matches(const struct opal_resp_tok *token, u8 match)
> +{
> +	if (IS_ERR_OR_NULL(token) ||
> +	    token->type != OPAL_DTA_TOKENID_TOKEN ||
> +	    token->pos[0] != match)
> +		return false;
> +	return true;
> +}
> +

This is sorta pedantic but from my reading of the code token can never be null. It will
either be a valid pointer or ERR_PTR(-EINVAL), but never null.
So maybe change this to IS_ERR(token) etc. Other than that little nit:

Reviewed-by: Scott Bauer <scott.bauer@intel.com>

[PATCHv3 3/4] block/sed: Check received header lengths

[Jon Derrick]

Add a buffer size check against discovery and response header lengths
before we loop over their buffers.

Signed-off-by: Jon Derrick <jonathan.derrick@intel.com>
---
 block/sed-opal.c | 35 +++++++++++++++++++++--------------
 1 file changed, 21 insertions(+), 14 deletions(-)

diff --git a/block/sed-opal.c b/block/sed-opal.c
index d6dd604..feba34b 100644
--- a/block/sed-opal.c
+++ b/block/sed-opal.c
@@ -340,10 +340,17 @@ static int opal_discovery0_end(struct opal_dev *dev)
 	const struct d0_header *hdr = (struct d0_header *)dev->resp;
 	const u8 *epos = dev->resp, *cpos = dev->resp;
 	u16 comid = 0;
+	u32 hlen = be32_to_cpu(hdr->length);
 
-	print_buffer(dev->resp, be32_to_cpu(hdr->length));
+	print_buffer(dev->resp, hlen);
 
-	epos += be32_to_cpu(hdr->length); /* end of buffer */
+	if (hlen > IO_BUFFER_LENGTH - sizeof(*hdr)) {
+		pr_warn("Discovery length overflows buffer (%zu+%u)/%u\n",
+			sizeof(*hdr), hlen, IO_BUFFER_LENGTH);
+		return -EFAULT;
+	}
+
+	epos += hlen; /* end of buffer */
 	cpos += sizeof(*hdr); /* current position on buffer */
 
 	while (cpos < epos && supported) {
@@ -713,6 +720,7 @@ static int response_parse(const u8 *buf, size_t length,
 	int total;
 	ssize_t token_length;
 	const u8 *pos;
+	u32 clen, plen, slen;
 
 	if (!buf)
 		return -EFAULT;
@@ -724,17 +732,16 @@ static int response_parse(const u8 *buf, size_t length,
 	pos = buf;
 	pos += sizeof(*hdr);
 
-	pr_debug("Response size: cp: %d, pkt: %d, subpkt: %d\n",
-		 be32_to_cpu(hdr->cp.length),
-		 be32_to_cpu(hdr->pkt.length),
-		 be32_to_cpu(hdr->subpkt.length));
-
-	if (hdr->cp.length == 0 || hdr->pkt.length == 0 ||
-	    hdr->subpkt.length == 0) {
-		pr_err("Bad header length. cp: %d, pkt: %d, subpkt: %d\n",
-		       be32_to_cpu(hdr->cp.length),
-		       be32_to_cpu(hdr->pkt.length),
-		       be32_to_cpu(hdr->subpkt.length));
+	clen = be32_to_cpu(hdr->cp.length);
+	plen = be32_to_cpu(hdr->pkt.length);
+	slen = be32_to_cpu(hdr->subpkt.length);
+	pr_debug("Response size: cp: %u, pkt: %u, subpkt: %u\n",
+		 clen, plen, slen);
+
+	if (clen == 0 || plen == 0 || slen == 0 ||
+	    slen > IO_BUFFER_LENGTH - sizeof(*hdr)) {
+		pr_err("Bad header length. cp: %u, pkt: %u, subpkt: %u\n",
+		       clen, plen, slen);
 		print_buffer(pos, sizeof(*hdr));
 		return -EINVAL;
 	}
@@ -743,7 +750,7 @@ static int response_parse(const u8 *buf, size_t length,
 		return -EFAULT;
 
 	iter = resp->toks;
-	total = be32_to_cpu(hdr->subpkt.length);
+	total = slen;
 	print_buffer(pos, total);
 	while (total > 0) {
 		if (pos[0] <= TINY_ATOM_BYTE) /* tiny atom */
-- 
1.8.3.1

[PATCHv3 3/4] block/sed: Check received header lengths

[Jon Derrick]

Add a buffer size check against discovery and response header lengths
before we loop over their buffers.

Signed-off-by: Jon Derrick <jonathan.derrick@intel.com>
---
 block/sed-opal.c | 35 +++++++++++++++++++++--------------
 1 file changed, 21 insertions(+), 14 deletions(-)

diff --git a/block/sed-opal.c b/block/sed-opal.c
index d6dd604..feba34b 100644
--- a/block/sed-opal.c
+++ b/block/sed-opal.c
@@ -340,10 +340,17 @@ static int opal_discovery0_end(struct opal_dev *dev)
 	const struct d0_header *hdr = (struct d0_header *)dev->resp;
 	const u8 *epos = dev->resp, *cpos = dev->resp;
 	u16 comid = 0;
+	u32 hlen = be32_to_cpu(hdr->length);
 
-	print_buffer(dev->resp, be32_to_cpu(hdr->length));
+	print_buffer(dev->resp, hlen);
 
-	epos += be32_to_cpu(hdr->length); /* end of buffer */
+	if (hlen > IO_BUFFER_LENGTH - sizeof(*hdr)) {
+		pr_warn("Discovery length overflows buffer (%zu+%u)/%u\n",
+			sizeof(*hdr), hlen, IO_BUFFER_LENGTH);
+		return -EFAULT;
+	}
+
+	epos += hlen; /* end of buffer */
 	cpos += sizeof(*hdr); /* current position on buffer */
 
 	while (cpos < epos && supported) {
@@ -713,6 +720,7 @@ static int response_parse(const u8 *buf, size_t length,
 	int total;
 	ssize_t token_length;
 	const u8 *pos;
+	u32 clen, plen, slen;
 
 	if (!buf)
 		return -EFAULT;
@@ -724,17 +732,16 @@ static int response_parse(const u8 *buf, size_t length,
 	pos = buf;
 	pos += sizeof(*hdr);
 
-	pr_debug("Response size: cp: %d, pkt: %d, subpkt: %d\n",
-		 be32_to_cpu(hdr->cp.length),
-		 be32_to_cpu(hdr->pkt.length),
-		 be32_to_cpu(hdr->subpkt.length));
-
-	if (hdr->cp.length == 0 || hdr->pkt.length == 0 ||
-	    hdr->subpkt.length == 0) {
-		pr_err("Bad header length. cp: %d, pkt: %d, subpkt: %d\n",
-		       be32_to_cpu(hdr->cp.length),
-		       be32_to_cpu(hdr->pkt.length),
-		       be32_to_cpu(hdr->subpkt.length));
+	clen = be32_to_cpu(hdr->cp.length);
+	plen = be32_to_cpu(hdr->pkt.length);
+	slen = be32_to_cpu(hdr->subpkt.length);
+	pr_debug("Response size: cp: %u, pkt: %u, subpkt: %u\n",
+		 clen, plen, slen);
+
+	if (clen == 0 || plen == 0 || slen == 0 ||
+	    slen > IO_BUFFER_LENGTH - sizeof(*hdr)) {
+		pr_err("Bad header length. cp: %u, pkt: %u, subpkt: %u\n",
+		       clen, plen, slen);
 		print_buffer(pos, sizeof(*hdr));
 		return -EINVAL;
 	}
@@ -743,7 +750,7 @@ static int response_parse(const u8 *buf, size_t length,
 		return -EFAULT;
 
 	iter = resp->toks;
-	total = be32_to_cpu(hdr->subpkt.length);
+	total = slen;
 	print_buffer(pos, total);
 	while (total > 0) {
 		if (pos[0] <= TINY_ATOM_BYTE) /* tiny atom */
-- 
1.8.3.1

Re: [PATCHv3 3/4] block/sed: Check received header lengths

[Scott Bauer]

On Wed, Feb 15, 2017 at 12:42:08PM -0700, Jon Derrick wrote:
> Add a buffer size check against discovery and response header lengths
> before we loop over their buffers.
> 
> Signed-off-by: Jon Derrick <jonathan.derrick@intel.com>
Reviewed-by: Scott Bauer <scott.bauer@intel.com>

[PATCHv3 4/4] MAINTAINERS: Remove powerpc's opal match

[Jon Derrick]

PPC's 'opal' match pattern also matches block/sed-opal.c, where it looks
like the 'arch/powerpc' file pattern should be enough to match powerpc
opal code by itself. Remove the opal regex pattern from powerpc.

Signed-off-by: Jon Derrick <jonathan.derrick@intel.com>
---
 MAINTAINERS | 1 -
 1 file changed, 1 deletion(-)

diff --git a/MAINTAINERS b/MAINTAINERS
index b983b25..430dd02 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -7404,7 +7404,6 @@ F:	drivers/pci/hotplug/pnv_php.c
 F:	drivers/pci/hotplug/rpa*
 F:	drivers/scsi/ibmvscsi/
 F:	tools/testing/selftests/powerpc
-N:	opal
 N:	/pmac
 N:	powermac
 N:	powernv
-- 
1.8.3.1

[PATCHv3 4/4] MAINTAINERS: Remove powerpc's opal match

[Jon Derrick]

PPC's 'opal' match pattern also matches block/sed-opal.c, where it looks
like the 'arch/powerpc' file pattern should be enough to match powerpc
opal code by itself. Remove the opal regex pattern from powerpc.

Signed-off-by: Jon Derrick <jonathan.derrick@intel.com>
---
 MAINTAINERS | 1 -
 1 file changed, 1 deletion(-)

diff --git a/MAINTAINERS b/MAINTAINERS
index b983b25..430dd02 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -7404,7 +7404,6 @@ F:	drivers/pci/hotplug/pnv_php.c
 F:	drivers/pci/hotplug/rpa*
 F:	drivers/scsi/ibmvscsi/
 F:	tools/testing/selftests/powerpc
-N:	opal
 N:	/pmac
 N:	powermac
 N:	powernv
-- 
1.8.3.1

Re: [PATCHv3 4/4] MAINTAINERS: Remove powerpc's opal match

[Stewart Smith]

Jon Derrick <jonathan.derrick@intel.com> writes:
> PPC's 'opal' match pattern also matches block/sed-opal.c, where it looks
> like the 'arch/powerpc' file pattern should be enough to match powerpc
> opal code by itself. Remove the opal regex pattern from powerpc.

This patch will end up missing some code, what about this instead:


Remove OPAL regex in powerpc to avoid false match

Signed-off-by: Stewart Smith <stewart@linux.vnet.ibm.com>

---
 MAINTAINERS |    8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/MAINTAINERS b/MAINTAINERS
index 3960e7f..25ed25a 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -7393,18 +7393,24 @@ L:	linuxppc-dev@lists.ozlabs.org
 Q:	http://patchwork.ozlabs.org/project/linuxppc-dev/list/
 T:	git git://git.kernel.org/pub/scm/linux/kernel/git/powerpc/linux.git
 S:	Supported
+F:	Documentation/ABI/stable/sysfs-firmware-opal-*
+F:	Documentation/devicetree/bindings/powerpc/opal/
+F:	Documentation/devicetree/bindings/rtc/rtc-opal.txt
+F:	Documentation/devicetree/bindings/i2c/i2c-opal.txt
 F:	Documentation/powerpc/
 F:	arch/powerpc/
 F:	drivers/char/tpm/tpm_ibmvtpm*
 F:	drivers/crypto/nx/
 F:	drivers/crypto/vmx/
+F:	drivers/i2c/busses/ic2-opal.c
 F:	drivers/net/ethernet/ibm/ibmveth.*
 F:	drivers/net/ethernet/ibm/ibmvnic.*
 F:	drivers/pci/hotplug/pnv_php.c
 F:	drivers/pci/hotplug/rpa*
+F:	drivers/rtc/rtc-opal.c
 F:	drivers/scsi/ibmvscsi/
+F:	drivers/tty/hvc/hvc_opal.c
 F:	tools/testing/selftests/powerpc
-N:	opal
 N:	/pmac
 N:	powermac
 N:	powernv


-- 
Stewart Smith
OPAL Architect, IBM.

Re: [PATCHv3 4/4] MAINTAINERS: Remove powerpc's opal match

[Andrew Donnellan]

On 16/02/17 11:37, Stewart Smith wrote:
> Jon Derrick <jonathan.derrick@intel.com> writes:
>> PPC's 'opal' match pattern also matches block/sed-opal.c, where it looks
>> like the 'arch/powerpc' file pattern should be enough to match powerpc
>> opal code by itself. Remove the opal regex pattern from powerpc.
>
> This patch will end up missing some code, what about this instead:
>
>
> Remove OPAL regex in powerpc to avoid false match
>
> Signed-off-by: Stewart Smith <stewart@linux.vnet.ibm.com>

Per my reply to v4 of this series, your list matches mine. With the one 
typo correction below:

Reviewed-by: Andrew Donnellan <andrew.donnellan@au1.ibm.com>

>
> ---
>  MAINTAINERS |    8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 3960e7f..25ed25a 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -7393,18 +7393,24 @@ L:	linuxppc-dev@lists.ozlabs.org
>  Q:	http://patchwork.ozlabs.org/project/linuxppc-dev/list/
>  T:	git git://git.kernel.org/pub/scm/linux/kernel/git/powerpc/linux.git
>  S:	Supported
> +F:	Documentation/ABI/stable/sysfs-firmware-opal-*
> +F:	Documentation/devicetree/bindings/powerpc/opal/
> +F:	Documentation/devicetree/bindings/rtc/rtc-opal.txt
> +F:	Documentation/devicetree/bindings/i2c/i2c-opal.txt
>  F:	Documentation/powerpc/
>  F:	arch/powerpc/
>  F:	drivers/char/tpm/tpm_ibmvtpm*
>  F:	drivers/crypto/nx/
>  F:	drivers/crypto/vmx/
> +F:	drivers/i2c/busses/ic2-opal.c

i2c-opal

>  F:	drivers/net/ethernet/ibm/ibmveth.*
>  F:	drivers/net/ethernet/ibm/ibmvnic.*
>  F:	drivers/pci/hotplug/pnv_php.c
>  F:	drivers/pci/hotplug/rpa*
> +F:	drivers/rtc/rtc-opal.c
>  F:	drivers/scsi/ibmvscsi/
> +F:	drivers/tty/hvc/hvc_opal.c
>  F:	tools/testing/selftests/powerpc
> -N:	opal
>  N:	/pmac
>  N:	powermac
>  N:	powernv
>
>

-- 
Andrew Donnellan              OzLabs, ADL Canberra
andrew.donnellan@au1.ibm.com  IBM Australia Limited

Re: [PATCHv3,4/4] MAINTAINERS: Remove powerpc's opal match

[Michael Ellerman]

On Thu, 2017-02-16 at 00:37:15 UTC, Stewart Smith wrote:
> Remove OPAL regex in powerpc to avoid false match
> 
> Signed-off-by: Stewart Smith <stewart@linux.vnet.ibm.com>
> Reviewed-by: Andrew Donnellan <andrew.donnellan@au1.ibm.com>

Applied to powerpc next, thanks.

https://git.kernel.org/powerpc/c/a42715830d552d7c0e3be709383ece

cheers

Re: [PATCHv3 4/4] MAINTAINERS: Remove powerpc's opal match

[Michael Ellerman]

Jon Derrick <jonathan.derrick@intel.com> writes:

> PPC's 'opal' match pattern also matches block/sed-opal.c, where it looks
> like the 'arch/powerpc' file pattern should be enough to match powerpc
> opal code by itself. Remove the opal regex pattern from powerpc.

We thought of it first.

Can't you just rename your driver, Opal Storage Specification, so "oss",
that should be pretty unique?

... :)

I don't like this version, but I'll merge the one from Stewart which
drops the pattern and adds the paths for the existing drivers.

cheers

> diff --git a/MAINTAINERS b/MAINTAINERS
> index b983b25..430dd02 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -7404,7 +7404,6 @@ F:	drivers/pci/hotplug/pnv_php.c
>  F:	drivers/pci/hotplug/rpa*
>  F:	drivers/scsi/ibmvscsi/
>  F:	tools/testing/selftests/powerpc
> -N:	opal
>  N:	/pmac
>  N:	powermac
>  N:	powernv
> -- 
> 1.8.3.1

Re: [PATCHv3 4/4] MAINTAINERS: Remove powerpc's opal match

[Jon Derrick]

Thanks everyone. Sorry about the mess :)

On 02/15/2017 10:23 PM, Michael Ellerman wrote:
> Jon Derrick <jonathan.derrick@intel.com> writes:
> 
>> PPC's 'opal' match pattern also matches block/sed-opal.c, where it looks
>> like the 'arch/powerpc' file pattern should be enough to match powerpc
>> opal code by itself. Remove the opal regex pattern from powerpc.
> 
> We thought of it first.
> 
> Can't you just rename your driver, Opal Storage Specification, so "oss",
> that should be pretty unique?
> 
> ... :)
> 
> I don't like this version, but I'll merge the one from Stewart which
> drops the pattern and adds the paths for the existing drivers.
> 
> cheers
> 
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index b983b25..430dd02 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -7404,7 +7404,6 @@ F:	drivers/pci/hotplug/pnv_php.c
>>  F:	drivers/pci/hotplug/rpa*
>>  F:	drivers/scsi/ibmvscsi/
>>  F:	tools/testing/selftests/powerpc
>> -N:	opal
>>  N:	/pmac
>>  N:	powermac
>>  N:	powernv
>> -- 
>> 1.8.3.1

RE: [PATCHv3 4/4] MAINTAINERS: Remove powerpc's opal match

[Elliott, Robert (Persistent Memory)]

> -----Original Message-----
> From: linux-block-owner@vger.kernel.org [mailto:linux-block-
> owner@vger.kernel.org] On Behalf Of Jon Derrick
> Sent: Thursday, February 16, 2017 10:15 AM
> To: Michael Ellerman <mpe@ellerman.id.au>
> Cc: Jens Axboe <axboe@kernel.dk>; Rafael Antognolli
> <rafael.antognolli@intel.com>; Greg Kroah-Hartman
> <gregkh@linuxfoundation.org>; linux-kernel@vger.kernel.org; linux-
> block@vger.kernel.org; linuxppc-dev@lists.ozlabs.org; Christoph Hellwig
> <hch@lst.de>; Scott Bauer <scott.bauer@intel.com>
> Subject: Re: [PATCHv3 4/4] MAINTAINERS: Remove powerpc's opal match
>=20
> Thanks everyone. Sorry about the mess :)
>=20
> On 02/15/2017 10:23 PM, Michael Ellerman wrote:
> > Jon Derrick <jonathan.derrick@intel.com> writes:
> >
> >> PPC's 'opal' match pattern also matches block/sed-opal.c, where it loo=
ks
> >> like the 'arch/powerpc' file pattern should be enough to match powerpc
> >> opal code by itself. Remove the opal regex pattern from powerpc.
> >
> > We thought of it first.
> >
> > Can't you just rename your driver, Opal Storage Specification, so "oss"=
,
> > that should be pretty unique?
> >
> > ... :)

The library could easily be used for devices supporting the Opalite and Pyr=
ite=20
SSCs, not just the Opal SSC. With some effort, I suspect that Enterprise SS=
C
could also be supported.  So, a broader name might indeed be useful.

The full names of the specifications are:
TCG Storage Security Subsystem Class: Opal
TCG Storage Security Subsystem Class: Opalite
TCG Storage Security Subsystem Class: Pyrite
TCG Storage Security Subsystem Class: Enterprise

---
Robert Elliott, HPE Persistent Memory

RE: [PATCHv3 4/4] MAINTAINERS: Remove powerpc's opal match

[Elliott, Robert (Persistent Memory)]

> -----Original Message-----
> From: linux-block-owner@vger.kernel.org [mailto:linux-block-
> owner@vger.kernel.org] On Behalf Of Jon Derrick
> Sent: Thursday, February 16, 2017 10:15 AM
> To: Michael Ellerman <mpe@ellerman.id.au>
> Cc: Jens Axboe <axboe@kernel.dk>; Rafael Antognolli
> <rafael.antognolli@intel.com>; Greg Kroah-Hartman
> <gregkh@linuxfoundation.org>; linux-kernel@vger.kernel.org; linux-
> block@vger.kernel.org; linuxppc-dev@lists.ozlabs.org; Christoph Hellwig
> <hch@lst.de>; Scott Bauer <scott.bauer@intel.com>
> Subject: Re: [PATCHv3 4/4] MAINTAINERS: Remove powerpc's opal match
> 
> Thanks everyone. Sorry about the mess :)
> 
> On 02/15/2017 10:23 PM, Michael Ellerman wrote:
> > Jon Derrick <jonathan.derrick@intel.com> writes:
> >
> >> PPC's 'opal' match pattern also matches block/sed-opal.c, where it looks
> >> like the 'arch/powerpc' file pattern should be enough to match powerpc
> >> opal code by itself. Remove the opal regex pattern from powerpc.
> >
> > We thought of it first.
> >
> > Can't you just rename your driver, Opal Storage Specification, so "oss",
> > that should be pretty unique?
> >
> > ... :)

The library could easily be used for devices supporting the Opalite and Pyrite 
SSCs, not just the Opal SSC. With some effort, I suspect that Enterprise SSC
could also be supported.  So, a broader name might indeed be useful.

The full names of the specifications are:
TCG Storage Security Subsystem Class: Opal
TCG Storage Security Subsystem Class: Opalite
TCG Storage Security Subsystem Class: Pyrite
TCG Storage Security Subsystem Class: Enterprise

---
Robert Elliott, HPE Persistent Memory