HI Sasha,<br><br>
<div class="gmail_quote">On Sun, Aug 2, 2009 at 6:32 AM, Sasha Khapyorsky <span dir="ltr"><<a href="mailto:sashak@voltaire.com" target="_blank">sashak@voltaire.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="PADDING-LEFT: 1ex; MARGIN: 0px 0px 0px 0.8ex; BORDER-LEFT: #ccc 1px solid">Hi Hal,<br>
<div><br>On 09:53 Fri 31 Jul , Hal Rosenstock wrote:<br>><br>> based on valid/invalid hop count rather than relying on debug assert<br><br></div>When could an invalid hop count be passed to this function? </blockquote>
<div> </div>
<div>Some out of tree user.</div>
<div> </div>
<blockquote class="gmail_quote" style="PADDING-LEFT: 1ex; MARGIN: 0px 0px 0px 0.8ex; BORDER-LEFT: #ccc 1px solid"><span id=""></span>And what could happen?</blockquote>
<div> </div>
<div>It writes past the end of the path array.</div>
<div> </div>
<blockquote class="gmail_quote" style="PADDING-LEFT: 1ex; MARGIN: 0px 0px 0px 0.8ex; BORDER-LEFT: #ccc 1px solid"><span id=""></span><br>ib_smp_init_new() is a simple structure fill-up helper (inlined and<br>defined in header file) and I don't think that we need to check<br>
parameters there.<br><br>This patch also introduces sort of inconsistency - a hop count is checked<br>and other parameters aren't.</blockquote>
<div> </div>
<div>It's to protect against writing past end of array. Do any other parameters need checking ? I think they just result in some timeout condition resulting.</div>
<div> </div>
<div>-- Hal</div>
<div> </div>
<blockquote class="gmail_quote" style="PADDING-LEFT: 1ex; MARGIN: 0px 0px 0px 0.8ex; BORDER-LEFT: #ccc 1px solid"><span id=""></span><br>
<div>
<div></div>
<div><br>> Handle invalid status appropriate in callers of ib_smp_init_new<br>><br>> Signed-off-by: Hal Rosenstock <<a href="mailto:hal.rosenstock@gmail.com" target="_blank">hal.rosenstock@gmail.com</a>><br>
> ---<br>> diff --git a/opensm/include/iba/ib_types.h b/opensm/include/iba/ib_types.h<br>> index beb7492..6668d96 100644<br>> --- a/opensm/include/iba/ib_types.h<br>> +++ b/opensm/include/iba/ib_types.h<br>
> @@ -4091,11 +4092,11 @@ static inline boolean_t OSM_API ib_smp_is_d(IN const ib_smp_t * const p_smp)<br>> *<br>> * TODO<br>> * This is too big for inlining, but leave it here for now<br>> -* since there is not yet another convient spot.<br>
> +* since there is not yet another convenient spot.<br>> *<br>> * SYNOPSIS<br>> */<br>> -static inline void OSM_API<br>> +static inline boolean_t OSM_API<br>> ib_smp_init_new(IN ib_smp_t * const p_smp,<br>
> IN const uint8_t method,<br>> IN const ib_net64_t trans_id,<br>> @@ -4107,7 +4108,9 @@ ib_smp_init_new(IN ib_smp_t * const p_smp,<br>> IN const ib_net16_t dr_slid, IN const ib_net16_t dr_dlid)<br>
> {<br>> CL_ASSERT(p_smp);<br>> - CL_ASSERT(hop_count < IB_SUBNET_PATH_HOPS_MAX);<br>> +<br>> + if (hop_count >= IB_SUBNET_PATH_HOPS_MAX)<br>> + return FALSE;<br>> p_smp->base_ver = 1;<br>
> p_smp->mgmt_class = IB_MCLASS_SUBN_DIR;<br>> p_smp->class_ver = 1;<br>> @@ -4130,6 +4133,7 @@ ib_smp_init_new(IN ib_smp_t * const p_smp,<br>><br>> /* copy the path */<br>> memcpy(&p_smp->initial_path, path_out, sizeof(p_smp->initial_path));<br>
> + return TRUE;<br>> }<br>><br>> /*<br>> diff --git a/opensm/opensm/osm_req.c b/opensm/opensm/osm_req.c<br>> index be9a92b..7934173 100644<br>> --- a/opensm/opensm/osm_req.c<br>> +++ b/opensm/opensm/osm_req.c<br>
> @@ -102,14 +102,21 @@ osm_req_get(IN osm_sm_t * sm,<br>> ib_get_sm_attr_str(attr_id), cl_ntoh16(attr_id),<br>> cl_ntoh32(attr_mod), cl_ntoh64(tid));<br>><br>> - ib_smp_init_new(osm_madw_get_smp_ptr(p_madw),<br>
> - IB_MAD_METHOD_GET,<br>> - tid,<br>> - attr_id,<br>> - attr_mod,<br>> - p_path->hop_count,<br>> - sm->p_subn->opt.m_key,<br>
> - p_path->path, IB_LID_PERMISSIVE, IB_LID_PERMISSIVE);<br>> + if (!ib_smp_init_new(osm_madw_get_smp_ptr(p_madw),<br>> + IB_MAD_METHOD_GET,<br>> + tid,<br>
> + attr_id,<br>> + attr_mod,<br>> + p_path->hop_count,<br>> + sm->p_subn->opt.m_key,<br>> + p_path->path,<br>
> + IB_LID_PERMISSIVE, IB_LID_PERMISSIVE)) {<br>> + OSM_LOG(sm->p_log, OSM_LOG_ERROR, "ERR 1108: "<br>> + "ib_smp_init_new failed: hop count %d\n",<br>
> + p_path->hop_count);<br><br></div></div>This is assumption on how ib_smp_init_new() is actually implemented -<br>not perfect.<br><font color="#888888"><br>Sasha<br></font>
<div>
<div></div>
<div>_______________________________________________<br>general mailing list<br><a href="mailto:general@lists.openfabrics.org" target="_blank">general@lists.openfabrics.org</a><br><a href="http://lists.openfabrics.org/cgi-bin/mailman/listinfo/general" target="_blank">http://lists.openfabrics.org/cgi-bin/mailman/listinfo/general</a><br>
<br>To unsubscribe, please visit <a href="http://openib.org/mailman/listinfo/openib-general" target="_blank">http://openib.org/mailman/listinfo/openib-general</a><br></div></div></blockquote></div><br>