| Message ID | 20220330150551.2573938-5-cristian.marussi@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | SCMIv3.1 Miscellaneous changes | expand |
On Wed, Mar 30, 2022 at 04:05:33PM +0100, Cristian Marussi wrote: > Do not blindly trust SCMI backend server reply about list of implemented > protocols, instead validate the reported length of the list of protocols > against the real payload size of the message reply. > > Fixes: b6f20ff8bd9 ("firmware: arm_scmi: add common infrastructure and support for base protocol") > Signed-off-by: Cristian Marussi <cristian.marussi@arm.com> > --- > drivers/firmware/arm_scmi/base.c | 21 +++++++++++++++++++++ > 1 file changed, 21 insertions(+) > > diff --git a/drivers/firmware/arm_scmi/base.c b/drivers/firmware/arm_scmi/base.c > index f279146f8110..c1165d1282ef 100644 > --- a/drivers/firmware/arm_scmi/base.c > +++ b/drivers/firmware/arm_scmi/base.c > @@ -189,6 +189,9 @@ scmi_base_implementation_list_get(const struct scmi_protocol_handle *ph, > list = t->rx.buf + sizeof(*num_ret); > > do { > + size_t real_list_sz; > + u32 calc_list_sz; > + > /* Set the number of protocols to be skipped/already read */ > *num_skip = cpu_to_le32(tot_num_ret); > > @@ -202,6 +205,24 @@ scmi_base_implementation_list_get(const struct scmi_protocol_handle *ph, > break; > } > > + if (t->rx.len < (sizeof(u32) * 2)) { > + dev_err(dev, "Truncated reply - rx.len:%zd\n", > + t->rx.len); > + ret = -EPROTO; > + break; > + } > + > + real_list_sz = t->rx.len - sizeof(u32); > + calc_list_sz = ((loop_num_ret / sizeof(u32)) + > + !!(loop_num_ret % sizeof(u32))) * sizeof(u32); Any reason this can't be (loop_num_ret - 1) / sizeof(u32) + 1 ? -- Regards, Sudeep
On Thu, Apr 28, 2022 at 11:07:29AM +0100, Sudeep Holla wrote: > On Wed, Mar 30, 2022 at 04:05:33PM +0100, Cristian Marussi wrote: > > Do not blindly trust SCMI backend server reply about list of implemented > > protocols, instead validate the reported length of the list of protocols > > against the real payload size of the message reply. > > > > Fixes: b6f20ff8bd9 ("firmware: arm_scmi: add common infrastructure and support for base protocol") > > Signed-off-by: Cristian Marussi <cristian.marussi@arm.com> > > --- > > drivers/firmware/arm_scmi/base.c | 21 +++++++++++++++++++++ > > 1 file changed, 21 insertions(+) > > > > diff --git a/drivers/firmware/arm_scmi/base.c b/drivers/firmware/arm_scmi/base.c > > index f279146f8110..c1165d1282ef 100644 > > --- a/drivers/firmware/arm_scmi/base.c > > +++ b/drivers/firmware/arm_scmi/base.c > > @@ -189,6 +189,9 @@ scmi_base_implementation_list_get(const struct scmi_protocol_handle *ph, > > list = t->rx.buf + sizeof(*num_ret); > > > > do { > > + size_t real_list_sz; > > + u32 calc_list_sz; > > + > > /* Set the number of protocols to be skipped/already read */ > > *num_skip = cpu_to_le32(tot_num_ret); > > > > @@ -202,6 +205,24 @@ scmi_base_implementation_list_get(const struct scmi_protocol_handle *ph, > > break; > > } > > > > + if (t->rx.len < (sizeof(u32) * 2)) { > > + dev_err(dev, "Truncated reply - rx.len:%zd\n", > > + t->rx.len); > > + ret = -EPROTO; > > + break; > > + } > > + > > + real_list_sz = t->rx.len - sizeof(u32); > > + calc_list_sz = ((loop_num_ret / sizeof(u32)) + > > + !!(loop_num_ret % sizeof(u32))) * sizeof(u32); > > Any reason this can't be (loop_num_ret - 1) / sizeof(u32) + 1 ? > At first sight could be fine with your easier version BUT what if loop_num_ret is returned as zero ? real_list_sz should be ZERO length and calc_list_sz im my version: calc_list_sz = ((0/4) +!!(0%4)) * 4 ===>> 0 while in the simplified one gets calculated wrong: calc_list_sz = (0-1)/4 + 1 ====> 1 ...moreover being both loop_num_ret and calc_list_sz unsigned I am even not so sure about implicit casting messing things up evenm more :D So I sticked to the more convoluted approach :D ....Have I missed something else ? Thanks, Cristian
On Thu, Apr 28, 2022 at 02:45:07PM +0100, Cristian Marussi wrote: > On Thu, Apr 28, 2022 at 11:07:29AM +0100, Sudeep Holla wrote: > > On Wed, Mar 30, 2022 at 04:05:33PM +0100, Cristian Marussi wrote: > > > Do not blindly trust SCMI backend server reply about list of implemented > > > protocols, instead validate the reported length of the list of protocols > > > against the real payload size of the message reply. > > > > > > Fixes: b6f20ff8bd9 ("firmware: arm_scmi: add common infrastructure and support for base protocol") > > > Signed-off-by: Cristian Marussi <cristian.marussi@arm.com> > > > --- > > > drivers/firmware/arm_scmi/base.c | 21 +++++++++++++++++++++ > > > 1 file changed, 21 insertions(+) > > > > > > diff --git a/drivers/firmware/arm_scmi/base.c b/drivers/firmware/arm_scmi/base.c > > > index f279146f8110..c1165d1282ef 100644 > > > --- a/drivers/firmware/arm_scmi/base.c > > > +++ b/drivers/firmware/arm_scmi/base.c > > > @@ -189,6 +189,9 @@ scmi_base_implementation_list_get(const struct scmi_protocol_handle *ph, > > > list = t->rx.buf + sizeof(*num_ret); > > > > > > do { > > > + size_t real_list_sz; > > > + u32 calc_list_sz; > > > + > > > /* Set the number of protocols to be skipped/already read */ > > > *num_skip = cpu_to_le32(tot_num_ret); > > > > > > @@ -202,6 +205,24 @@ scmi_base_implementation_list_get(const struct scmi_protocol_handle *ph, > > > break; > > > } > > > > > > + if (t->rx.len < (sizeof(u32) * 2)) { > > > + dev_err(dev, "Truncated reply - rx.len:%zd\n", > > > + t->rx.len); > > > + ret = -EPROTO; > > > + break; > > > + } > > > + > > > + real_list_sz = t->rx.len - sizeof(u32); > > > + calc_list_sz = ((loop_num_ret / sizeof(u32)) + > > > + !!(loop_num_ret % sizeof(u32))) * sizeof(u32); > > > > Any reason this can't be (loop_num_ret - 1) / sizeof(u32) + 1 ? > > > > At first sight could be fine with your easier version BUT what if loop_num_ret > is returned as zero ? > > real_list_sz should be ZERO length and calc_list_sz > > im my version: > > calc_list_sz = ((0/4) +!!(0%4)) * 4 ===>> 0 > > while in the simplified one gets calculated wrong: > > calc_list_sz = (0-1)/4 + 1 ====> 1 > > ...moreover being both loop_num_ret and calc_list_sz unsigned I am even > not so sure about implicit casting messing things up evenm more :D > > So I sticked to the more convoluted approach :D > > ....Have I missed something else ? > Right, but shouldn't we break if it 0 much earlier. It must not happen with your new logic and even if it does, wouldn't it be better to break earlier ?
On Thu, Apr 28, 2022 at 02:55:04PM +0100, Sudeep Holla wrote: > On Thu, Apr 28, 2022 at 02:45:07PM +0100, Cristian Marussi wrote: > > On Thu, Apr 28, 2022 at 11:07:29AM +0100, Sudeep Holla wrote: > > > On Wed, Mar 30, 2022 at 04:05:33PM +0100, Cristian Marussi wrote: > > > > Do not blindly trust SCMI backend server reply about list of implemented > > > > protocols, instead validate the reported length of the list of protocols > > > > against the real payload size of the message reply. > > > > > > > > Fixes: b6f20ff8bd9 ("firmware: arm_scmi: add common infrastructure and support for base protocol") > > > > Signed-off-by: Cristian Marussi <cristian.marussi@arm.com> > > > > --- > > > > drivers/firmware/arm_scmi/base.c | 21 +++++++++++++++++++++ > > > > 1 file changed, 21 insertions(+) > > > > > > > > diff --git a/drivers/firmware/arm_scmi/base.c b/drivers/firmware/arm_scmi/base.c > > > > index f279146f8110..c1165d1282ef 100644 > > > > --- a/drivers/firmware/arm_scmi/base.c > > > > +++ b/drivers/firmware/arm_scmi/base.c > > > > @@ -189,6 +189,9 @@ scmi_base_implementation_list_get(const struct scmi_protocol_handle *ph, > > > > list = t->rx.buf + sizeof(*num_ret); > > > > > > > > do { > > > > + size_t real_list_sz; > > > > + u32 calc_list_sz; > > > > + > > > > /* Set the number of protocols to be skipped/already read */ > > > > *num_skip = cpu_to_le32(tot_num_ret); > > > > > > > > @@ -202,6 +205,24 @@ scmi_base_implementation_list_get(const struct scmi_protocol_handle *ph, > > > > break; > > > > } > > > > > > > > + if (t->rx.len < (sizeof(u32) * 2)) { > > > > + dev_err(dev, "Truncated reply - rx.len:%zd\n", > > > > + t->rx.len); > > > > + ret = -EPROTO; > > > > + break; > > > > + } > > > > + > > > > + real_list_sz = t->rx.len - sizeof(u32); > > > > + calc_list_sz = ((loop_num_ret / sizeof(u32)) + > > > > + !!(loop_num_ret % sizeof(u32))) * sizeof(u32); > > > > > > Any reason this can't be (loop_num_ret - 1) / sizeof(u32) + 1 ? > > > > > > > At first sight could be fine with your easier version BUT what if loop_num_ret > > is returned as zero ? > > > > real_list_sz should be ZERO length and calc_list_sz > > > > im my version: > > > > calc_list_sz = ((0/4) +!!(0%4)) * 4 ===>> 0 > > > > while in the simplified one gets calculated wrong: > > > > calc_list_sz = (0-1)/4 + 1 ====> 1 > > > > ...moreover being both loop_num_ret and calc_list_sz unsigned I am even > > not so sure about implicit casting messing things up evenm more :D > > > > So I sticked to the more convoluted approach :D > > > > ....Have I missed something else ? > > > > Right, but shouldn't we break if it 0 much earlier. It must not happen with > your new logic and even if it does, wouldn't it be better to break earlier ? > Fine for me. Thanks, Cristian
diff --git a/drivers/firmware/arm_scmi/base.c b/drivers/firmware/arm_scmi/base.c index f279146f8110..c1165d1282ef 100644 --- a/drivers/firmware/arm_scmi/base.c +++ b/drivers/firmware/arm_scmi/base.c @@ -189,6 +189,9 @@ scmi_base_implementation_list_get(const struct scmi_protocol_handle *ph, list = t->rx.buf + sizeof(*num_ret); do { + size_t real_list_sz; + u32 calc_list_sz; + /* Set the number of protocols to be skipped/already read */ *num_skip = cpu_to_le32(tot_num_ret); @@ -202,6 +205,24 @@ scmi_base_implementation_list_get(const struct scmi_protocol_handle *ph, break; } + if (t->rx.len < (sizeof(u32) * 2)) { + dev_err(dev, "Truncated reply - rx.len:%zd\n", + t->rx.len); + ret = -EPROTO; + break; + } + + real_list_sz = t->rx.len - sizeof(u32); + calc_list_sz = ((loop_num_ret / sizeof(u32)) + + !!(loop_num_ret % sizeof(u32))) * sizeof(u32); + if (calc_list_sz != real_list_sz) { + dev_err(dev, + "Malformed reply - real_sz:%zd calc_sz:%u\n", + real_list_sz, calc_list_sz); + ret = -EPROTO; + break; + } + for (loop = 0; loop < loop_num_ret; loop++) protocols_imp[tot_num_ret + loop] = *(list + loop);
Do not blindly trust SCMI backend server reply about list of implemented protocols, instead validate the reported length of the list of protocols against the real payload size of the message reply. Fixes: b6f20ff8bd9 ("firmware: arm_scmi: add common infrastructure and support for base protocol") Signed-off-by: Cristian Marussi <cristian.marussi@arm.com> --- drivers/firmware/arm_scmi/base.c | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+)