Patchwork Patch for review

login
register
mail settings
Submitter Rahul Jain
Date Feb. 21, 2014, 11:36 a.m.
Message ID <01.3C.09100.92A37035@epcpsbgx2.samsung.com>
Download mbox | patch
Permalink /patch/322563/
State Changes Requested
Headers show

Comments

Rahul Jain - Feb. 21, 2014, 11:36 a.m.
hi,

Please review the attached patch.


From c5311fb4154d81907d23cd9998af09b205f18525 Mon Sep 17 00:00:00 2001
From: Rahul Jain <rahul.jain@samsung.com>

Date: Fri, 21 Feb 2014 17:03:18 +0530
Subject: [PATCH] Fix for checking go intent value before going ahead for
 processing of Go Neg Request Frame


Signed-off-by: Rahul Jain <rahul.jain@samsung.com>

---
 src/p2p/p2p_go_neg.c |    8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

-- 
1.7.9.5

Thanks
-Rahul Jain
Jouni Malinen - Feb. 21, 2014, 1:17 p.m.
On Fri, Feb 21, 2014 at 11:36:09AM +0000, Rahul Jain wrote:
> Please review the attached patch.

> Subject: [PATCH] Fix for checking go intent value before going ahead for
>  processing of Go Neg Request Frame

Please note that it would be useful to use this subject line from the
commit as the subject of the message as well.

> diff --git a/src/p2p/p2p_go_neg.c b/src/p2p/p2p_go_neg.c
> @@ -545,8 +545,14 @@ void p2p_process_go_neg_req(struct p2p_data *p2p, const u8 *sa,
>  #endif /* CONFIG_P2P_STRICT */
>  	}
>  
> -	if (msg.go_intent)
> +	if (msg.go_intent) {
>  		tie_breaker = *msg.go_intent & 0x01;
> +		if ((*msg.go_intent >> 1) > P2P_MAX_GO_INTENT) {
> +			p2p_dbg(p2p, "Invalid GO Intent value (%u) received",
> +				*msg.go_intent >> 1);
> +			goto fail;
> +		}

While the change itself would be a correct validation step, isn't this
already done in the function when GO Negotiation would be allowed to
start? As such, this addition here would have very little change in the
behavior (status code would change for couple of error paths). Is there
a specific case where this is needed? If so, I think I would be fine
with the change, but I would combine it with removing the other check
that would become practically dead code if this is added into the
beginning of the function.

Patch

diff --git a/src/p2p/p2p_go_neg.c b/src/p2p/p2p_go_neg.c

index 2e40db1..c6b47be 100644

--- a/src/p2p/p2p_go_neg.c

+++ b/src/p2p/p2p_go_neg.c

@@ -545,8 +545,14 @@  void p2p_process_go_neg_req(struct p2p_data *p2p, const u8 *sa,

 #endif /* CONFIG_P2P_STRICT */
 	}
 
-	if (msg.go_intent)

+	if (msg.go_intent) {

 		tie_breaker = *msg.go_intent & 0x01;
+		if ((*msg.go_intent >> 1) > P2P_MAX_GO_INTENT) {

+			p2p_dbg(p2p, "Invalid GO Intent value (%u) received",

+				*msg.go_intent >> 1);

+			goto fail;

+		}

+	}

 	else {
 		p2p_dbg(p2p, "Mandatory GO Intent attribute missing from GO Negotiation Request");
 #ifdef CONFIG_P2P_STRICT