Message ID | 1474186483-30831-1-git-send-email-Yuval.Mintz@qlogic.com |
---|---|
State | Changes Requested, archived |
Delegated to: | David Miller |
Headers | show |
From: Yuval Mintz > Sent: 18 September 2016 09:15 > Commit fe56b9e6a8d95 ("qed: Add module with basic common support") > has introduced a stack corruption during probe, where filling a > local struct with data to be sent to management firmware is incorrectly > filled; The data is written outside of the struct and corrupts > the stack. > > Fixes: fe56b9e6a8d95 ("qed: Add module with basic common support") > Signed-off-by: Yuval Mintz <Yuval.Mintz@caviumnetworks.com> > --- > Hi Dave, > > In case it isn't obvious at first glance, the corruption is due > to the next line in the for-loop, which isn't changed by the patch. > > Please consider applying this to `net'. > > Thanks, > Yuval > --- > drivers/net/ethernet/qlogic/qed/qed_mcp.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/net/ethernet/qlogic/qed/qed_mcp.c b/drivers/net/ethernet/qlogic/qed/qed_mcp.c > index a240f26..69f5b04 100644 > --- a/drivers/net/ethernet/qlogic/qed/qed_mcp.c > +++ b/drivers/net/ethernet/qlogic/qed/qed_mcp.c > @@ -1153,8 +1153,8 @@ qed_mcp_send_drv_version(struct qed_hwfn *p_hwfn, > p_drv_version = &union_data.drv_version; > p_drv_version->version = p_ver->version; > > - for (i = 0; i < MCP_DRV_VER_STR_SIZE - 1; i += 4) { > - val = cpu_to_be32(p_ver->name[i]); > + for (i = 0; i < (MCP_DRV_VER_STR_SIZE - 4) / sizeof(u32); i++) { > + val = cpu_to_be32(p_ver->name[i * sizeof(u32)]); > *(__be32 *)&p_drv_version->name[i * sizeof(u32)] = val; > } That change fails the 'sanity' test. It looks like it is copying a string that might by byteswapped into host order. I'm guessing that the target p_drv_version->name[] is char []. The cpu_to_be32() call is only correct if p_ver->name[] is 32bit. But you are (now) indexing it with the character offset. So the data copied cannot be right. I also suspect it should be be32_to_cpu(). David
> > - for (i = 0; i < MCP_DRV_VER_STR_SIZE - 1; i += 4) { > > - val = cpu_to_be32(p_ver->name[i]); > > + for (i = 0; i < (MCP_DRV_VER_STR_SIZE - 4) / sizeof(u32); i++) { > > + val = cpu_to_be32(p_ver->name[i * sizeof(u32)]); > > *(__be32 *)&p_drv_version->name[i * sizeof(u32)] = val; > > } > > That change fails the 'sanity' test. > It looks like it is copying a string that might by byteswapped into host order. > I'm guessing that the target p_drv_version->name[] is char []. > The cpu_to_be32() call is only correct if p_ver->name[] is 32bit. > But you are (now) indexing it with the character offset. > So the data copied cannot be right. Crap. Thanks for catching this up; I'll send v2 shortly. > I also suspect it should be be32_to_cpu(). Actually, the conversion is correct as-is - data is provided by driver at host order and should be sent to management firmware as BE.
diff --git a/drivers/net/ethernet/qlogic/qed/qed_mcp.c b/drivers/net/ethernet/qlogic/qed/qed_mcp.c index a240f26..69f5b04 100644 --- a/drivers/net/ethernet/qlogic/qed/qed_mcp.c +++ b/drivers/net/ethernet/qlogic/qed/qed_mcp.c @@ -1153,8 +1153,8 @@ qed_mcp_send_drv_version(struct qed_hwfn *p_hwfn, p_drv_version = &union_data.drv_version; p_drv_version->version = p_ver->version; - for (i = 0; i < MCP_DRV_VER_STR_SIZE - 1; i += 4) { - val = cpu_to_be32(p_ver->name[i]); + for (i = 0; i < (MCP_DRV_VER_STR_SIZE - 4) / sizeof(u32); i++) { + val = cpu_to_be32(p_ver->name[i * sizeof(u32)]); *(__be32 *)&p_drv_version->name[i * sizeof(u32)] = val; }
Commit fe56b9e6a8d95 ("qed: Add module with basic common support") has introduced a stack corruption during probe, where filling a local struct with data to be sent to management firmware is incorrectly filled; The data is written outside of the struct and corrupts the stack. Fixes: fe56b9e6a8d95 ("qed: Add module with basic common support") Signed-off-by: Yuval Mintz <Yuval.Mintz@caviumnetworks.com> --- Hi Dave, In case it isn't obvious at first glance, the corruption is due to the next line in the for-loop, which isn't changed by the patch. Please consider applying this to `net'. Thanks, Yuval --- drivers/net/ethernet/qlogic/qed/qed_mcp.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)