On 30.08.21 12:54, Z.Q. Hou wrote: > > >> -----Original Message----- >> From: Jan Kiszka [mailto:[email protected]] >> Sent: 2021年8月30日 13:43 >> To: Z.Q. Hou <[email protected]>; [email protected] >> Subject: Re: [PATCH] arm: irqchip: Fix the mask according to access address >> and >> size >> >> On 30.08.21 05:02, Z.Q. Hou wrote: >>> Hi Jan, >>> >>>> -----Original Message----- >>>> From: Jan Kiszka [mailto:[email protected]] >>>> Sent: 2021年8月27日 15:53 >>>> To: Z.Q. Hou <[email protected]>; [email protected] >>>> Subject: Re: [PATCH] arm: irqchip: Fix the mask according to access >>>> address and size >>>> >>>> On 27.08.21 09:51, Zhiqiang Hou wrote: >>>>> From: Hou Zhiqiang <[email protected]> >>>>> >>>>> In function restrict_bitmask_access(), the current access_mask is >>>>> implicitly assuming that it always access the whole register, but >>>>> some registers are byte-accessible, the Guest may get/set wrong >>>>> value when it issue a byte or halfword access to these registers. >>>> >>>> Can you be more specific in the affected scenarios? At least one >>>> example would be good. >>> >>> Let take GICD_IPRIORITYR as an example, which is a byte-accessible register. >>> Thinking the SPI INTID 33 is assigned to an inmate cell, and Guest wants to >>> set >> the priority of 33 by byte-accessing. >>> >>> Assuming: >>> GICD_IPRIORITY8 original value: 0x00000000 Guest set value: 0xa0 >>> >>> Then the current logic like: >>> The access_mask is calculated as 0xff00, GICD_IPRIORITY8 read back value is >> 0x00. Then 0x00 & ~(0xff00) get 0x00, 0x00 | (0xa0 & 0xff00) get 0x00, then >> finally 0x00 will be written back. >>> >>> With this fix, the access_mask will become (0xff00 >> 8) & 0xff = 0xff, so >>> finally >> the 0xa0 will be written back. >>> >> >> OK, indeed a good point. We missed the byte-accessability of GICD_IPRIORITYR. >> >> But this reveals more: We perform a read-modify-write cycle on behalf of the >> cell >> with the cell's idea of access width. That is potentially unsafe. > > Can you help understand why there is still potential unsafe with this fix > patch? >
The patch is fine for GICD_IPRIORITYR, so is accepting all accesses for that register. The problem is with other registers that do not support byte accesses. So, to make things simpler, we convert to always performing word accesses for all of them. That means no extra effort for the other regs, just filtering of size != 4, but GICD_IPRIORITYR now needs upfront work. And that provides the chance to address also the issue you found. >> >> Handling of GICD_ITARGETSR has to resolve the same issue, and it does that by >> converting the request unconditionally into a word-access. I think we should >> do >> the same here as well. > > I think this fix way is more simple than converting to word-access > unconditionally, as this fix way only tweaks the access_mask, while the > word-access way need to tweak both the addr and value. > We could stack the two changes. I'll take yours first, and then we only need to decide who does the second fix on top... Jan -- Siemens AG, T RDA IOT Corporate Competence Center Embedded Linux -- You received this message because you are subscribed to the Google Groups "Jailhouse" group. To unsubscribe from this group and stop receiving emails from it, send an email to [email protected]. To view this discussion on the web visit https://groups.google.com/d/msgid/jailhouse-dev/9ecc5d87-c7b4-ce84-6621-f88cf05fbeff%40siemens.com.
