iommufd: Use sizeof(*hdr) instead of sizeof(hdr) in veventq read

PATCH Applied Kai Aizen 5 participants 5 messages Apr 30 – May 8

Subsystems (3)

Stable branch IOMMU Iommufd

lore

Share & tools

lore

Kai Aizen

author

2026-04-30 17:56 UTC #0 lore raw

PATCH v2 1/1 iommufd: Use sizeof(*hdr) instead of sizeof(hdr) in veventq read

The bound-check in iommufd_veventq_fops_read() for the normal vEVENT path uses sizeof(hdr) where the surrounding code uses sizeof(*hdr):

	if (!vevent_for_lost_events_header(cur) &&
	    sizeof(hdr) + cur->data_len > count - done) {

hdr is declared as struct iommufd_vevent_header *, so sizeof(hdr) evaluates to the size of the pointer. Surrounding code uses sizeof(*hdr) consistently:

	if (done >= count || sizeof(*hdr) > count - done) {
	...
	if (copy_to_user(buf + done, hdr, sizeof(*hdr))) {
	...
	done += sizeof(*hdr);

struct iommufd_vevent_header is currently 8 bytes (two __u32 fields, flags and sequence), so on 64-bit (sizeof(void *) == 8) the two expressions happen to be equal and the check works as intended.

On 32-bit (sizeof(void *) == 4) the check under-counts the header by 4 bytes: a vEVENT whose data_len causes 8 + cur->data_len to exceed count - done while 4 + cur->data_len does not will pass the check, then the loop will copy_to_user 8 bytes of header followed by data_len bytes of payload, writing past the user-supplied buffer.

It is also a latent bug for any future expansion of struct iommufd_vevent_header beyond sizeof(void *) on 64-bit; the check should not depend on the type happening to match the host pointer width.

Use sizeof(*hdr) to match the rest of the function and the actual amount that will be copied.

Fixes e36ba5ab808e ("iommufd: Add IOMMUFD_OBJ_VEVENTQ and IOMMUFD_CMD_VEVENTQ_ALLOC")

Cc [email protected]

Reported-by Kai Aizen <[email protected]>

Signed-off-by Kai Aizen <[email protected]>

---
v2: fix From/Signed-off-by to use real name and email address.

---
 drivers/iommu/iommufd/eventq.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/iommu/iommufd/eventq.c +1 -1

index 710eef0b6..78689fb52 100644
@@ -321,7 +321,7 @@ static ssize_t iommufd_veventq_fops_read(struct file *filep, char __user *buf,
 
 		/* If being a normal vEVENT, validate against the full size */
 		if (!vevent_for_lost_events_header(cur) &&
-		    sizeof(hdr) + cur->data_len > count - done) {
+		    sizeof(*hdr) + cur->data_len > count - done) {
 			iommufd_veventq_deliver_restore(veventq, cur);
 			break;
 		}

Signature · 1 line

2.43.0

Baolu Lu

Reviewed-by on v2 1/1

2026-05-05 03:26 UTC #1 lore raw

Quoted 31 lines · Kai Aizen

The bound-check in iommufd_veventq_fops_read() for the normal vEVENT path uses sizeof(hdr) where the surrounding code uses sizeof(*hdr):
	if (!vevent_for_lost_events_header(cur) &&
	    sizeof(hdr) + cur->data_len > count - done) {
hdr is declared as struct iommufd_vevent_header *, so sizeof(hdr) evaluates to the size of the pointer. Surrounding code uses sizeof(*hdr) consistently:
	if (done >= count || sizeof(*hdr) > count - done) {
	...
	if (copy_to_user(buf + done, hdr, sizeof(*hdr))) {
	...
	done += sizeof(*hdr);
struct iommufd_vevent_header is currently 8 bytes (two __u32 fields, flags and sequence), so on 64-bit (sizeof(void *) == 8) the two expressions happen to be equal and the check works as intended.
On 32-bit (sizeof(void *) == 4) the check under-counts the header by 4 bytes: a vEVENT whose data_len causes 8 + cur->data_len to exceed count - done while 4 + cur->data_len does not will pass the check, then the loop will copy_to_user 8 bytes of header followed by data_len bytes of payload, writing past the user-supplied buffer.
It is also a latent bug for any future expansion of struct iommufd_vevent_header beyond sizeof(void *) on 64-bit; the check should not depend on the type happening to match the host pointer width.

Quoted 7 lines

Use sizeof(*hdr) to match the rest of the function and the actual amount that will be copied.
Fixes e36ba5ab808e ("iommufd: Add IOMMUFD_OBJ_VEVENTQ and IOMMUFD_CMD_VEVENTQ_ALLOC")
Cc [email protected]
Reported-by Kai Aizen<[email protected]>
Signed-off-by Kai Aizen<[email protected]>

Reviewed-by Lu Baolu <[email protected]>

Nicolin Chen

Reviewed-by on v2 1/1

2026-05-05 03:46 UTC #2 lore raw

In reply to Kai Aizen

Quoted 31 lines · Kai Aizen

The bound-check in iommufd_veventq_fops_read() for the normal vEVENT path uses sizeof(hdr) where the surrounding code uses sizeof(*hdr):
	if (!vevent_for_lost_events_header(cur) &&
	    sizeof(hdr) + cur->data_len > count - done) {
hdr is declared as struct iommufd_vevent_header *, so sizeof(hdr) evaluates to the size of the pointer. Surrounding code uses sizeof(*hdr) consistently:
	if (done >= count || sizeof(*hdr) > count - done) {
	...
	if (copy_to_user(buf + done, hdr, sizeof(*hdr))) {
	...
	done += sizeof(*hdr);
struct iommufd_vevent_header is currently 8 bytes (two __u32 fields, flags and sequence), so on 64-bit (sizeof(void *) == 8) the two expressions happen to be equal and the check works as intended.
On 32-bit (sizeof(void *) == 4) the check under-counts the header by 4 bytes: a vEVENT whose data_len causes 8 + cur->data_len to exceed count - done while 4 + cur->data_len does not will pass the check, then the loop will copy_to_user 8 bytes of header followed by data_len bytes of payload, writing past the user-supplied buffer.
It is also a latent bug for any future expansion of struct iommufd_vevent_header beyond sizeof(void *) on 64-bit; the check should not depend on the type happening to match the host pointer width.

Quoted 7 lines

Use sizeof(*hdr) to match the rest of the function and the actual amount that will be copied.
Fixes e36ba5ab808e ("iommufd: Add IOMMUFD_OBJ_VEVENTQ and IOMMUFD_CMD_VEVENTQ_ALLOC")
Cc [email protected]
Reported-by Kai Aizen <[email protected]>
Signed-off-by Kai Aizen <[email protected]>

Reviewed-by Nicolin Chen <[email protected]>

Tian, Kevin

maintainer Reviewed-by on v2 1/1

2026-05-08 08:17 UTC #3 lore raw

In reply to Kai Aizen

Quoted 34 lines

From: Kai Aizen <[email protected]>
Sent: Friday, May 1, 2026 1:57 AM
The bound-check in iommufd_veventq_fops_read() for the normal vEVENT path uses sizeof(hdr) where the surrounding code uses sizeof(*hdr):
	if (!vevent_for_lost_events_header(cur) &&
	    sizeof(hdr) + cur->data_len > count - done) {
hdr is declared as struct iommufd_vevent_header *, so sizeof(hdr) evaluates to the size of the pointer. Surrounding code uses sizeof(*hdr) consistently:
	if (done >= count || sizeof(*hdr) > count - done) {
	...
	if (copy_to_user(buf + done, hdr, sizeof(*hdr))) {
	...
	done += sizeof(*hdr);
struct iommufd_vevent_header is currently 8 bytes (two __u32 fields, flags and sequence), so on 64-bit (sizeof(void *) == 8) the two expressions happen to be equal and the check works as intended.
On 32-bit (sizeof(void *) == 4) the check under-counts the header by 4 bytes: a vEVENT whose data_len causes 8 + cur->data_len to exceed count - done while 4 + cur->data_len does not will pass the check, then the loop will copy_to_user 8 bytes of header followed by data_len bytes of payload, writing past the user-supplied buffer.
It is also a latent bug for any future expansion of struct iommufd_vevent_header beyond sizeof(void *) on 64-bit; the check should not depend on the type happening to match the host pointer width.

Quoted 8 lines

Use sizeof(*hdr) to match the rest of the function and the actual amount that will be copied.
Fixes e36ba5ab808e ("iommufd: Add IOMMUFD_OBJ_VEVENTQ and
IOMMUFD_CMD_VEVENTQ_ALLOC")
Cc [email protected]
Reported-by Kai Aizen <[email protected]>
Signed-off-by Kai Aizen <[email protected]>

Reviewed-by Kevin Tian <[email protected]>

Jason Gunthorpe

maintainer Applied

2026-05-08 23:37 UTC #4 lore raw

In reply to Kai Aizen

Quoted 38 lines · Kai Aizen

The bound-check in iommufd_veventq_fops_read() for the normal vEVENT path uses sizeof(hdr) where the surrounding code uses sizeof(*hdr):
	if (!vevent_for_lost_events_header(cur) &&
	    sizeof(hdr) + cur->data_len > count - done) {
hdr is declared as struct iommufd_vevent_header *, so sizeof(hdr) evaluates to the size of the pointer. Surrounding code uses sizeof(*hdr) consistently:
	if (done >= count || sizeof(*hdr) > count - done) {
	...
	if (copy_to_user(buf + done, hdr, sizeof(*hdr))) {
	...
	done += sizeof(*hdr);
struct iommufd_vevent_header is currently 8 bytes (two __u32 fields, flags and sequence), so on 64-bit (sizeof(void *) == 8) the two expressions happen to be equal and the check works as intended.
On 32-bit (sizeof(void *) == 4) the check under-counts the header by 4 bytes: a vEVENT whose data_len causes 8 + cur->data_len to exceed count - done while 4 + cur->data_len does not will pass the check, then the loop will copy_to_user 8 bytes of header followed by data_len bytes of payload, writing past the user-supplied buffer.
It is also a latent bug for any future expansion of struct iommufd_vevent_header beyond sizeof(void *) on 64-bit; the check should not depend on the type happening to match the host pointer width.
Use sizeof(*hdr) to match the rest of the function and the actual amount that will be copied.
Fixes e36ba5ab808e ("iommufd: Add IOMMUFD_OBJ_VEVENTQ and IOMMUFD_CMD_VEVENTQ_ALLOC")
Cc [email protected]
Reported-by Kai Aizen <[email protected]>
Signed-off-by Kai Aizen <[email protected]>

Quoted 5 lines

---
v2: fix From/Signed-off-by to use real name and email address.
---
 drivers/iommu/iommufd/eventq.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Applied to for-rc, thanks

Jason