[Intel-wired-lan] [Patch net-queue] igb: Correct vf reset response size

[Intel-wired-lan] [Patch net-queue] igb: Correct vf reset response size

[Tony Nguyen]

From: Akihiko Odaki <akihiko.odaki@daynix.com>

When an address is not assigned to a VF, sending E1000_VF_RESET message
from the VF to the PF causes the kernel to leak 64 bits on the stack to
its response, potentially crossing the security boundary relying on
virtualization.

Correct vf reset response size so that it does not leak data.

Fixes: 6ddbc4cf1f4d ("igb: Indicate failure on vf reset for empty mac address")
Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
---
 drivers/net/ethernet/intel/igb/igb_main.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c
index a5049e342f7d..74694a27e35d 100644
--- a/drivers/net/ethernet/intel/igb/igb_main.c
+++ b/drivers/net/ethernet/intel/igb/igb_main.c
@@ -7577,10 +7577,11 @@ static void igb_vf_reset_msg(struct igb_adapter *adapter, u32 vf)
 	if (!is_zero_ether_addr(vf_mac)) {
 		msgbuf[0] = E1000_VF_RESET | E1000_VT_MSGTYPE_ACK;
 		memcpy(addr, vf_mac, ETH_ALEN);
+		igb_write_mbx(hw, msgbuf, 3, vf);
 	} else {
 		msgbuf[0] = E1000_VF_RESET | E1000_VT_MSGTYPE_NACK;
+		igb_write_mbx(hw, msgbuf, 1, vf);
 	}
-	igb_write_mbx(hw, msgbuf, 3, vf);
 }
 
 static void igb_flush_mac_table(struct igb_adapter *adapter)
-- 
2.35.1

_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan

Re: [Intel-wired-lan] [Patch net-queue] igb: Correct vf reset response size

[Tony Nguyen]

On 12/5/2022 9:37 AM, Tony Nguyen wrote:
> From: Akihiko Odaki <akihiko.odaki@daynix.com>
> 
> When an address is not assigned to a VF, sending E1000_VF_RESET message
> from the VF to the PF causes the kernel to leak 64 bits on the stack to
> its response, potentially crossing the security boundary relying on
> virtualization.
> 
> Correct vf reset response size so that it does not leak data.
> 
> Fixes: 6ddbc4cf1f4d ("igb: Indicate failure on vf reset for empty mac address")
> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
> ---
>   drivers/net/ethernet/intel/igb/igb_main.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c
> index a5049e342f7d..74694a27e35d 100644
> --- a/drivers/net/ethernet/intel/igb/igb_main.c
> +++ b/drivers/net/ethernet/intel/igb/igb_main.c
> @@ -7577,10 +7577,11 @@ static void igb_vf_reset_msg(struct igb_adapter *adapter, u32 vf)
>   	if (!is_zero_ether_addr(vf_mac)) {
>   		msgbuf[0] = E1000_VF_RESET | E1000_VT_MSGTYPE_ACK;
>   		memcpy(addr, vf_mac, ETH_ALEN);
> +		igb_write_mbx(hw, msgbuf, 3, vf);
>   	} else {
>   		msgbuf[0] = E1000_VF_RESET | E1000_VT_MSGTYPE_NACK;
> +		igb_write_mbx(hw, msgbuf, 1, vf);

I just noticed this, but I believe the mailbox response needs to stay a 
consistent size as the VF is reading a set size of 3[1] for the op. We 
could probably zero init instead to prevent any leaking.

Thanks,
Tony

[1] 
https://elixir.bootlin.com/linux/latest/source/drivers/net/ethernet/intel/igbvf/vf.c#L132
_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan

Re: [Intel-wired-lan] [Patch net-queue] igb: Correct vf reset response size

[Tony Nguyen]

Specifically adding Akihiko to response.

On 12/5/2022 11:11 AM, Tony Nguyen wrote:
> On 12/5/2022 9:37 AM, Tony Nguyen wrote:
>> From: Akihiko Odaki <akihiko.odaki@daynix.com>
>>
>> When an address is not assigned to a VF, sending E1000_VF_RESET message
>> from the VF to the PF causes the kernel to leak 64 bits on the stack to
>> its response, potentially crossing the security boundary relying on
>> virtualization.
>>
>> Correct vf reset response size so that it does not leak data.
>>
>> Fixes: 6ddbc4cf1f4d ("igb: Indicate failure on vf reset for empty mac 
>> address")
>> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
>> ---
>>   drivers/net/ethernet/intel/igb/igb_main.c | 3 ++-
>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/net/ethernet/intel/igb/igb_main.c 
>> b/drivers/net/ethernet/intel/igb/igb_main.c
>> index a5049e342f7d..74694a27e35d 100644
>> --- a/drivers/net/ethernet/intel/igb/igb_main.c
>> +++ b/drivers/net/ethernet/intel/igb/igb_main.c
>> @@ -7577,10 +7577,11 @@ static void igb_vf_reset_msg(struct 
>> igb_adapter *adapter, u32 vf)
>>       if (!is_zero_ether_addr(vf_mac)) {
>>           msgbuf[0] = E1000_VF_RESET | E1000_VT_MSGTYPE_ACK;
>>           memcpy(addr, vf_mac, ETH_ALEN);
>> +        igb_write_mbx(hw, msgbuf, 3, vf);
>>       } else {
>>           msgbuf[0] = E1000_VF_RESET | E1000_VT_MSGTYPE_NACK;
>> +        igb_write_mbx(hw, msgbuf, 1, vf);
> 
> I just noticed this, but I believe the mailbox response needs to stay a 
> consistent size as the VF is reading a set size of 3[1] for the op. We 
> could probably zero init instead to prevent any leaking.
> 
> Thanks,
> Tony
> 
> [1] 
> https://elixir.bootlin.com/linux/latest/source/drivers/net/ethernet/intel/igbvf/vf.c#L132
> _______________________________________________
> Intel-wired-lan mailing list
> Intel-wired-lan@osuosl.org
> https://lists.osuosl.org/mailman/listinfo/intel-wired-lan
_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan