On Wed, Mar 17, 2021 at 9:04 PM Gustavo A. R. Silva
<[email protected]> wrote:
> On 3/17/21 13:57, Jann Horn wrote:
> >>>> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_common.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_common.c
> >>>> index 62ddb452f862..bff3dc1af702 100644
> >>>> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_common.c
> >>>> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_common.c
> >>>> @@ -3679,7 +3679,7 @@ s32 ixgbe_host_interface_command(struct ixgbe_hw *hw, void *buffer,
> >>>> u32 hdr_size = sizeof(struct ixgbe_hic_hdr);
> >>>> union {
> >>>> struct ixgbe_hic_hdr hdr;
> >>>> - u32 u32arr[1];
> >>>> + u32 *u32arr;
> >>>> } *bp = buffer;
> >>>> u16 buf_len, dword_len;
> >>>> s32 status;
> >>>
> >>> This looks bogus. An array is inline, a pointer points elsewhere -
> >>> they're not interchangeable.
> >>
> >> Yep; but in this case these are the only places in the code where _u32arr_ is
> >> being used:
> >>
> >> 3707 /* first pull in the header so we know the buffer length */
> >> 3708 for (bi = 0; bi < dword_len; bi++) {
> >> 3709 bp->u32arr[bi] = IXGBE_READ_REG_ARRAY(hw, IXGBE_FLEX_MNG, bi);
> >> 3710 le32_to_cpus(&bp->u32arr[bi]);
> >> 3711 }
> >
> > So now line 3709 means: Read a pointer from bp->u32arr (the value
> > being read from there is not actually a valid pointer), and write to
> > that pointer at offset `bi`. I don't see how that line could execute
> > without crashing.
>
> Yeah; you're right. I see my confusion now. Apparently, there is no escape
> from allocating heap memory to fix this issue, as I was proposing in my
> last email.
Why? Can't you do something like this?
diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_common.c
b/drivers/net/ethernet/intel/ixgbe/ixgbe_common.c
index 62ddb452f862..768fa124105b 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_common.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_common.c
@@ -3677,10 +3677,8 @@ s32 ixgbe_host_interface_command(struct
ixgbe_hw *hw, void *buffer,
bool return_data)
{
u32 hdr_size = sizeof(struct ixgbe_hic_hdr);
- union {
- struct ixgbe_hic_hdr hdr;
- u32 u32arr[1];
- } *bp = buffer;
+ u32 *bp = buffer;
+ struct ixgbe_hic_hdr hdr;
u16 buf_len, dword_len;
s32 status;
u32 bi;
@@ -3706,12 +3704,13 @@ s32 ixgbe_host_interface_command(struct
ixgbe_hw *hw, void *buffer,
/* first pull in the header so we know the buffer length */
for (bi = 0; bi < dword_len; bi++) {
- bp->u32arr[bi] = IXGBE_READ_REG_ARRAY(hw, IXGBE_FLEX_MNG, bi);
- le32_to_cpus(&bp->u32arr[bi]);
+ bp[bi] = IXGBE_READ_REG_ARRAY(hw, IXGBE_FLEX_MNG, bi);
+ le32_to_cpus(&bp[bi]);
}
/* If there is any thing in data position pull it in */
- buf_len = bp->hdr.buf_len;
+ memcpy(&hdr, bp, sizeof(hdr));
+ buf_len = hdr.buf_len;
if (!buf_len)
goto rel_out;
@@ -3726,8 +3725,8 @@ s32 ixgbe_host_interface_command(struct ixgbe_hw
*hw, void *buffer,
/* Pull in the rest of the buffer (bi is where we left off) */
for (; bi <= dword_len; bi++) {
- bp->u32arr[bi] = IXGBE_READ_REG_ARRAY(hw, IXGBE_FLEX_MNG, bi);
- le32_to_cpus(&bp->u32arr[bi]);
+ bp[bi] = IXGBE_READ_REG_ARRAY(hw, IXGBE_FLEX_MNG, bi);
+ le32_to_cpus(&bp[bi]);
}
rel_out:
On 3/17/21 15:10, Jann Horn wrote:
> On Wed, Mar 17, 2021 at 9:04 PM Gustavo A. R. Silva
> <[email protected]> wrote:
>> On 3/17/21 13:57, Jann Horn wrote:
>>>>>> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_common.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_common.c
>>>>>> index 62ddb452f862..bff3dc1af702 100644
>>>>>> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_common.c
>>>>>> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_common.c
>>>>>> @@ -3679,7 +3679,7 @@ s32 ixgbe_host_interface_command(struct ixgbe_hw *hw, void *buffer,
>>>>>> u32 hdr_size = sizeof(struct ixgbe_hic_hdr);
>>>>>> union {
>>>>>> struct ixgbe_hic_hdr hdr;
>>>>>> - u32 u32arr[1];
>>>>>> + u32 *u32arr;
>>>>>> } *bp = buffer;
>>>>>> u16 buf_len, dword_len;
>>>>>> s32 status;
>>>>>
>>>>> This looks bogus. An array is inline, a pointer points elsewhere -
>>>>> they're not interchangeable.
>>>>
>>>> Yep; but in this case these are the only places in the code where _u32arr_ is
>>>> being used:
>>>>
>>>> 3707 /* first pull in the header so we know the buffer length */
>>>> 3708 for (bi = 0; bi < dword_len; bi++) {
>>>> 3709 bp->u32arr[bi] = IXGBE_READ_REG_ARRAY(hw, IXGBE_FLEX_MNG, bi);
>>>> 3710 le32_to_cpus(&bp->u32arr[bi]);
>>>> 3711 }
>>>
>>> So now line 3709 means: Read a pointer from bp->u32arr (the value
>>> being read from there is not actually a valid pointer), and write to
>>> that pointer at offset `bi`. I don't see how that line could execute
>>> without crashing.
>>
>> Yeah; you're right. I see my confusion now. Apparently, there is no escape
>> from allocating heap memory to fix this issue, as I was proposing in my
>> last email.
>
> Why? Can't you do something like this?
Yep; it seems you're right. I was thinking in terms of a flexible array. Also,
I think I needed more coffee in my system this morning and I need to stop
working after midnight. :)
I'll send a proper patch for this, shortly. I'll add your Proposed-by
and Co-developed-by tags to the changelog text.
Thanks a lot for the feedback. I really appreciate it. :)
--
Gustavo
>
> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_common.c
> b/drivers/net/ethernet/intel/ixgbe/ixgbe_common.c
> index 62ddb452f862..768fa124105b 100644
> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_common.c
> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_common.c
> @@ -3677,10 +3677,8 @@ s32 ixgbe_host_interface_command(struct
> ixgbe_hw *hw, void *buffer,
> bool return_data)
> {
> u32 hdr_size = sizeof(struct ixgbe_hic_hdr);
> - union {
> - struct ixgbe_hic_hdr hdr;
> - u32 u32arr[1];
> - } *bp = buffer;
> + u32 *bp = buffer;
> + struct ixgbe_hic_hdr hdr;
> u16 buf_len, dword_len;
> s32 status;
> u32 bi;
> @@ -3706,12 +3704,13 @@ s32 ixgbe_host_interface_command(struct
> ixgbe_hw *hw, void *buffer,
>
> /* first pull in the header so we know the buffer length */
> for (bi = 0; bi < dword_len; bi++) {
> - bp->u32arr[bi] = IXGBE_READ_REG_ARRAY(hw, IXGBE_FLEX_MNG, bi);
> - le32_to_cpus(&bp->u32arr[bi]);
> + bp[bi] = IXGBE_READ_REG_ARRAY(hw, IXGBE_FLEX_MNG, bi);
> + le32_to_cpus(&bp[bi]);
> }
>
> /* If there is any thing in data position pull it in */
> - buf_len = bp->hdr.buf_len;
> + memcpy(&hdr, bp, sizeof(hdr));
> + buf_len = hdr.buf_len;
> if (!buf_len)
> goto rel_out;
>
> @@ -3726,8 +3725,8 @@ s32 ixgbe_host_interface_command(struct ixgbe_hw
> *hw, void *buffer,
>
> /* Pull in the rest of the buffer (bi is where we left off) */
> for (; bi <= dword_len; bi++) {
> - bp->u32arr[bi] = IXGBE_READ_REG_ARRAY(hw, IXGBE_FLEX_MNG, bi);
> - le32_to_cpus(&bp->u32arr[bi]);
> + bp[bi] = IXGBE_READ_REG_ARRAY(hw, IXGBE_FLEX_MNG, bi);
> + le32_to_cpus(&bp[bi]);
> }
>
> rel_out:
>