Hi, Michal.
I did not build that code. I was just looking through the output of
cppcheck and did not want to ignore a complaint that appeared to be
real and not part of an upstream package. Also, I think my previous
patch would generate a compiler warning about passing a const as a
non-const pointer, so I have attached an updated patch (which I also
hereby release under the terms of the MIT license).
Adam
Adam
On Mon, Apr 29, 2019 at 11:20 AM Michal Necasek
<[email protected]> wrote:
>
>
> Thanks for the patch. Just out of curiosity, did you check if the code is
> ever compiled in VirtualBox? I very much doubt we ever shipped that
> particular iPXE driver or ever intended to, and users would have to work
> quite hard to make the corresponding hardware appear in a VirtualBox VM.
>
>
> Regards,
> Michal
>
> ----- Original Message -----
> From: [email protected]
> To: [email protected]
> Sent: Friday, April 26, 2019 9:46:10 PM GMT +01:00 Amsterdam / Berlin / Bern
> / Rome / Stockholm / Vienna
> Subject: [vbox-dev] Untested patch: suspected memory fault in
> falcon_clear_interrupts()
>
> Hi.
>
> Running cppcheck on VirtualBox sources pointed me to what I think is a
> bug on falcon_clear_interrupts() will get a null pointer dereference
> is the PCI revision is not FALON_REV_B0. I have attached a proposed
> patch and also appended that patch inline for easier reading.
>
> I am sorry that I have not even attempted to see if the patch compiles
> because I don't think I have time to do a VirtualBox build until at
> least the weekend. As far as I know, I do not actually run code in
> this file. Also, I have not been involved in contributing to
> VirtualBox development, so please excuse any violations of contributor
> conventions and please feel free to let me know how I can do better
> next time.
>
> If this patch compiles and there are no complaints, I request that you
> merge this patch or another fix for this issue more to your liking. I
> hereby grant the permissions of the "MIT license" for any copyright
> interest I may have in this patch. Please let me know if you need me
> to make any other statement so that you can merge this patch.
>
> Adam Richter
>
>
> Index: src/VBox/Devices/PC/ipxe/src/drivers/net/etherfabric.c
> ===================================================================
> --- src/VBox/Devices/PC/ipxe/src/drivers/net/etherfabric.c (revision
> 78203)
> +++ src/VBox/Devices/PC/ipxe/src/drivers/net/etherfabric.c (working copy)
> @@ -3798,7 +3798,8 @@
> }
> else {
> /* write to the INT_ACK register */
> - falcon_writel ( efab, 0, FCN_INT_ACK_KER_REG_A1 );
> + static const efab_dword_t zero;
> + falcon_writel ( efab, &zero, FCN_INT_ACK_KER_REG_A1 );
> mb();
> falcon_readl ( efab, ®,
> WORK_AROUND_BROKEN_PCI_READS_REG_KER_A1 );
>
> _______________________________________________
> vbox-dev mailing list
> [email protected]
> https://www.virtualbox.org/mailman/listinfo/vbox-dev
Index: src/VBox/Devices/PC/ipxe/src/drivers/net/etherfabric.c
===================================================================
--- src/VBox/Devices/PC/ipxe/src/drivers/net/etherfabric.c (revision 78203)
+++ src/VBox/Devices/PC/ipxe/src/drivers/net/etherfabric.c (working copy)
@@ -1235,7 +1235,8 @@
*
*/
static inline void
-falcon_writel ( struct efab_nic *efab, efab_dword_t *value, unsigned int reg )
+falcon_writel ( struct efab_nic *efab, const efab_dword_t *value,
+ unsigned int reg )
{
EFAB_REGDUMP ( "Writing partial register %x with " EFAB_DWORD_FMT "\n",
reg, EFAB_DWORD_VAL ( *value ) );
@@ -3798,7 +3799,8 @@
}
else {
/* write to the INT_ACK register */
- falcon_writel ( efab, 0, FCN_INT_ACK_KER_REG_A1 );
+ static const efab_dword_t zero;
+ falcon_writel ( efab, &zero, FCN_INT_ACK_KER_REG_A1 );
mb();
falcon_readl ( efab, ®,
WORK_AROUND_BROKEN_PCI_READS_REG_KER_A1 );
_______________________________________________
vbox-dev mailing list
[email protected]
https://www.virtualbox.org/mailman/listinfo/vbox-dev
