[U-Boot] [PATCH v3] arm: socfpga: mailbox: Fix off-by-one error on command length checking
A mailbox command contains 1-DWORD header + arguments. The "len" variable only contains the length of the arguments, but not the 1-DWORD header. Include the length of header when checking the ring buffer space to prevent off-by-one error.
Signed-off-by: Ley Foon Tan ley.foon.tan@intel.com Signed-off-by: Chee Hong Ang chee.hong.ang@intel.com --- v2->v3: - Update commit description. --- arch/arm/mach-socfpga/mailbox_s10.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/arm/mach-socfpga/mailbox_s10.c b/arch/arm/mach-socfpga/mailbox_s10.c index 3c33223936..8363c93e90 100644 --- a/arch/arm/mach-socfpga/mailbox_s10.c +++ b/arch/arm/mach-socfpga/mailbox_s10.c @@ -59,7 +59,7 @@ static __always_inline int mbox_fill_cmd_circular_buff(u32 header, u32 len, */ if (((cin + 1) % MBOX_CMD_BUFFER_SIZE) == cout || ((MBOX_CMD_BUFFER_SIZE - cin + cout - 1) % - MBOX_CMD_BUFFER_SIZE) < len) + MBOX_CMD_BUFFER_SIZE) < (len + 1)) return -ENOMEM;
/* write header to circular buffer */
On 4/19/19 8:17 AM, Ley Foon Tan wrote:
A mailbox command contains 1-DWORD header + arguments. The "len" variable only contains the length of the arguments, but not the 1-DWORD header. Include the length of header when checking the ring buffer space to prevent off-by-one error.
How long is a DWORD ? Windows API (which we have nothing to do with) defines that as 32bit type, "typedef unsigned long DWORD;", see [1]. But the patch below fixes an off-by-one error , not off by four error ?
[1] https://docs.microsoft.com/en-us/windows/desktop/winprog/windows-data-types
Signed-off-by: Ley Foon Tan ley.foon.tan@intel.com Signed-off-by: Chee Hong Ang chee.hong.ang@intel.com
v2->v3:
- Update commit description.
arch/arm/mach-socfpga/mailbox_s10.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/arm/mach-socfpga/mailbox_s10.c b/arch/arm/mach-socfpga/mailbox_s10.c index 3c33223936..8363c93e90 100644 --- a/arch/arm/mach-socfpga/mailbox_s10.c +++ b/arch/arm/mach-socfpga/mailbox_s10.c @@ -59,7 +59,7 @@ static __always_inline int mbox_fill_cmd_circular_buff(u32 header, u32 len, */ if (((cin + 1) % MBOX_CMD_BUFFER_SIZE) == cout || ((MBOX_CMD_BUFFER_SIZE - cin + cout - 1) %
MBOX_CMD_BUFFER_SIZE) < len)
MBOX_CMD_BUFFER_SIZE) < (len + 1))return -ENOMEM;
/* write header to circular buffer */
Marek Vasut marex@denx.de schrieb am Fr., 19. Apr. 2019, 11:29:
On 4/19/19 8:17 AM, Ley Foon Tan wrote:
A mailbox command contains 1-DWORD header + arguments. The "len" variable only contains the length of the arguments, but not the 1-DWORD header. Include the length of header when checking the ring buffer space to prevent off-by-one error.
How long is a DWORD ? Windows API (which we have nothing to do with) defines that as 32bit type, "typedef unsigned long DWORD;", see [1]. But the patch below fixes an off-by-one error , not off by four error ?
As all the macros for that mailbox seem to do u32 index access only, I'd be ok with the commit message if it didn't use the term 'DWORD'...
Regards, Simon
[1] https://docs.microsoft.com/en-us/windows/desktop/winprog/windows-data-types
Signed-off-by: Ley Foon Tan ley.foon.tan@intel.com Signed-off-by: Chee Hong Ang chee.hong.ang@intel.com
v2->v3:
- Update commit description.
arch/arm/mach-socfpga/mailbox_s10.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/arm/mach-socfpga/mailbox_s10.c
b/arch/arm/mach-socfpga/mailbox_s10.c
index 3c33223936..8363c93e90 100644 --- a/arch/arm/mach-socfpga/mailbox_s10.c +++ b/arch/arm/mach-socfpga/mailbox_s10.c @@ -59,7 +59,7 @@ static __always_inline int
mbox_fill_cmd_circular_buff(u32 header, u32 len,
*/ if (((cin + 1) % MBOX_CMD_BUFFER_SIZE) == cout || ((MBOX_CMD_BUFFER_SIZE - cin + cout - 1) %
MBOX_CMD_BUFFER_SIZE) < len)
MBOX_CMD_BUFFER_SIZE) < (len + 1)) return -ENOMEM; /* write header to circular buffer */-- Best regards, Marek Vasut
On 4/19/19 10:52 PM, Simon Goldschmidt wrote:
Marek Vasut <marex@denx.de mailto:marex@denx.de> schrieb am Fr., 19. Apr. 2019, 11:29:
On 4/19/19 8:17 AM, Ley Foon Tan wrote: > A mailbox command contains 1-DWORD header + arguments. The "len" variable > only contains the length of the arguments, but not the 1-DWORD header. > Include the length of header when checking the ring buffer space to > prevent off-by-one error. How long is a DWORD ? Windows API (which we have nothing to do with) defines that as 32bit type, "typedef unsigned long DWORD;", see [1]. But the patch below fixes an off-by-one error , not off by four error ?As all the macros for that mailbox seem to do u32 index access only, I'd be ok with the commit message if it didn't use the term 'DWORD'...
Does that mean MBOX_CMD_BUFFER_SIZE is not in byte units, but in u32 units ?
Regards, Simon
[1] https://docs.microsoft.com/en-us/windows/desktop/winprog/windows-data-types > Signed-off-by: Ley Foon Tan <ley.foon.tan@intel.com <mailto:ley.foon.tan@intel.com>> > Signed-off-by: Chee Hong Ang <chee.hong.ang@intel.com <mailto:chee.hong.ang@intel.com>> > --- > v2->v3: > - Update commit description. > --- > arch/arm/mach-socfpga/mailbox_s10.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/arch/arm/mach-socfpga/mailbox_s10.c b/arch/arm/mach-socfpga/mailbox_s10.c > index 3c33223936..8363c93e90 100644 > --- a/arch/arm/mach-socfpga/mailbox_s10.c > +++ b/arch/arm/mach-socfpga/mailbox_s10.c > @@ -59,7 +59,7 @@ static __always_inline int mbox_fill_cmd_circular_buff(u32 header, u32 len, > */ > if (((cin + 1) % MBOX_CMD_BUFFER_SIZE) == cout || > ((MBOX_CMD_BUFFER_SIZE - cin + cout - 1) % > - MBOX_CMD_BUFFER_SIZE) < len) > + MBOX_CMD_BUFFER_SIZE) < (len + 1)) > return -ENOMEM; > > /* write header to circular buffer */ > -- Best regards, Marek Vasut
On Sun, Apr 21, 2019 at 4:30 PM Marek Vasut marex@denx.de wrote:
On 4/19/19 10:52 PM, Simon Goldschmidt wrote:
Marek Vasut <marex@denx.de mailto:marex@denx.de> schrieb am Fr., 19. Apr. 2019, 11:29:
On 4/19/19 8:17 AM, Ley Foon Tan wrote: > A mailbox command contains 1-DWORD header + arguments. The "len" variable > only contains the length of the arguments, but not the 1-DWORD header. > Include the length of header when checking the ring buffer space to > prevent off-by-one error. How long is a DWORD ? Windows API (which we have nothing to do with) defines that as 32bit type, "typedef unsigned long DWORD;", see [1]. But the patch below fixes an off-by-one error , not off by four error ?As all the macros for that mailbox seem to do u32 index access only, I'd be ok with the commit message if it didn't use the term 'DWORD'...
Does that mean MBOX_CMD_BUFFER_SIZE is not in byte units, but in u32 units ?
Yes, it is in u32 unit. One slot location is u32. How about change DWORD to u32 in commit message?
Regards Ley Foon
On 4/22/19 3:20 AM, Ley Foon Tan wrote:
On Sun, Apr 21, 2019 at 4:30 PM Marek Vasut marex@denx.de wrote:
On 4/19/19 10:52 PM, Simon Goldschmidt wrote:
Marek Vasut <marex@denx.de mailto:marex@denx.de> schrieb am Fr., 19. Apr. 2019, 11:29:
On 4/19/19 8:17 AM, Ley Foon Tan wrote: > A mailbox command contains 1-DWORD header + arguments. The "len" variable > only contains the length of the arguments, but not the 1-DWORD header. > Include the length of header when checking the ring buffer space to > prevent off-by-one error. How long is a DWORD ? Windows API (which we have nothing to do with) defines that as 32bit type, "typedef unsigned long DWORD;", see [1]. But the patch below fixes an off-by-one error , not off by four error ?As all the macros for that mailbox seem to do u32 index access only, I'd be ok with the commit message if it didn't use the term 'DWORD'...
Does that mean MBOX_CMD_BUFFER_SIZE is not in byte units, but in u32 units ?
Yes, it is in u32 unit. One slot location is u32. How about change DWORD to u32 in commit message?
Yes please. It would also make sense to add a comment into the code that len is not in bytes, but u32 units .
participants (4)
-
Ley Foon Tan -
Ley Foon Tan -
Marek Vasut -
Simon Goldschmidt