Message ID | 01.3C.09100.92A37035@epcpsbgx2.samsung.com |
---|---|
State | Changes Requested |
Headers | show |
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.
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