| Message ID | trinity-7c1b2e82-e34f-4885-8060-2cd7a13769ce-1623532166177@3c-app-gmx-bs52 |
|---|---|
| State | New |
| Headers | show |
| Series | can: bcm: fix infoleak in struct bcm_msg_head | expand |
>Hi, > >1. >Are you sure this leak really happens on 64-bit and not on 32-bit instead? > >I remember I got the problems with bcm msg head on the 32bit raspberry >pi because I missed the alignment by accident. > >When I calculate the size of msg head on a Ryzen 1800X with Python >3.9.5, I get: > >struct.calcsize("IIIllllII"),struct.calcsize("IIIllllII0q") >(56, 56) > >First Value is raw, the second value is the alignment hack with the zero >length quad word "0q". > >On the 32bit raspberry pi, same op results in the gap. > >struct.calcsize("IIIllllII"),struct.calcsize("IIIllllII0q") >(36, 40) Hey Patrick, having reproduced this leak I could only observe the issue on 64-bit systems. I've just tested it on a 32-bit OS running on a raspberry pi and I couldn't observe any leak. The offset difference on 32-bit between count and ival1 is 4. On 64-bit systems, it's 8: (gdb) ptype struct bcm_msg_head type = struct bcm_msg_head { __u32 opcode; __u32 flags; __u32 count; struct bcm_timeval ival1; struct bcm_timeval ival2; canid_t can_id; __u32 nframes; struct can_frame frames[0]; } (gdb) p/x &((struct bcm_msg_head *)0x0)->count $1 = 0x8 (gdb) p/x &((struct bcm_msg_head *)0x0)->ival1 $2 = 0x10 (gdb) p sizeof(((struct bcm_msg_head *)0x0)->count) $3 = 4 >2. >Finding stucts with non-zero-ed gaps should be easy with a skript or >even better with a GCC directive. I believe Syzbot does such a thing too. > >Kind Regards, >Patrick Menschel I didn't notice any syzbot report about this leak, nor did I find it with syzkaller. Norbert
Am 13.06.21 um 15:35 schrieb Norbert Slusarek: >> Hi, >> >> 1. >> Are you sure this leak really happens on 64-bit and not on 32-bit instead? >> >> I remember I got the problems with bcm msg head on the 32bit raspberry >> pi because I missed the alignment by accident. >> >> When I calculate the size of msg head on a Ryzen 1800X with Python >> 3.9.5, I get: >> >> struct.calcsize("IIIllllII"),struct.calcsize("IIIllllII0q") >> (56, 56) >> >> First Value is raw, the second value is the alignment hack with the zero >> length quad word "0q". >> >> On the 32bit raspberry pi, same op results in the gap. >> >> struct.calcsize("IIIllllII"),struct.calcsize("IIIllllII0q") >> (36, 40) > > Hey Patrick, > > having reproduced this leak I could only observe the issue on 64-bit systems. > I've just tested it on a 32-bit OS running on a raspberry pi and I couldn't observe > any leak. The offset difference on 32-bit between count and ival1 is 4. > On 64-bit systems, it's 8: > > (gdb) ptype struct bcm_msg_head > type = struct bcm_msg_head { > __u32 opcode; > __u32 flags; > __u32 count; > struct bcm_timeval ival1; > struct bcm_timeval ival2; > canid_t can_id; > __u32 nframes; > struct can_frame frames[0]; > } > (gdb) p/x &((struct bcm_msg_head *)0x0)->count > $1 = 0x8 > (gdb) p/x &((struct bcm_msg_head *)0x0)->ival1 > $2 = 0x10 > (gdb) p sizeof(((struct bcm_msg_head *)0x0)->count) > $3 = 4 > Ouch, I should not skip lines while reading. We're talking about different gaps as it seems. I didn't realize the gap in front of ival1 before. There is also a gap in between nframes and frames[0]. That one is caused by align(8) of data in struct can_frame. It propagates upwards into that gap on 32bit arch. You can find it if you actually fill frames[] with a frame. I found it while concatenating bcm_msg_head and a can frame into a python bytearray which was too short for the raspberry pi as I forgot the alignment. I came up with a format string "IIIllllII0q" for bcm_msg_head. Kind Regards, Patrick
On 12.06.2021 23:09:26, Norbert Slusarek wrote: > From: Norbert Slusarek <nslusarek@gmx.net> > Date: Sat, 12 Jun 2021 22:18:54 +0200 > Subject: [PATCH] can: bcm: fix infoleak in struct bcm_msg_head > > On 64-bit systems, struct bcm_msg_head has an added padding of 4 bytes between > struct members count and ival1. Even though all struct members are initialized, > the 4-byte hole will contain data from the kernel stack. This patch zeroes out > struct bcm_msg_head before usage, preventing infoleaks to userspace. > > Fixes: ffd980f976e7 ("[CAN]: Add broadcast manager (bcm) protocol") > Signed-off-by: Norbert Slusarek <nslusarek@gmx.net> Added to linux-can/testing. regards, Marc P.S.: I think the gmx web interface mangled the patch and converted tabs to spaces. Try to use git send-mail to avoid this. -- Pengutronix e.K. | Marc Kleine-Budde | Embedded Linux | https://www.pengutronix.de | Vertretung West/Dortmund | Phone: +49-231-2826-924 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
The issue has been assigned CVE-2021-34693 and the announcement on oss-security is available in the link below. https://www.openwall.com/lists/oss-security/2021/06/15/1 Norbert
diff --git a/net/can/bcm.c b/net/can/bcm.c index 909b9e684e04..b03062f84fe7 100644 --- a/net/can/bcm.c +++ b/net/can/bcm.c @@ -402,6 +402,7 @@ static enum hrtimer_restart bcm_tx_timeout_handler(struct hrtimer *hrtimer) if (!op->count && (op->flags & TX_COUNTEVT)) { /* create notification to user */ + memset(&msg_head, 0, sizeof(msg_head)); msg_head.opcode = TX_EXPIRED; msg_head.flags = op->flags; msg_head.count = op->count; @@ -439,6 +440,7 @@ static void bcm_rx_changed(struct bcm_op *op, struct canfd_frame *data) /* this element is not throttled anymore */ data->flags &= (BCM_CAN_FLAGS_MASK|RX_RECV); + memset(&head, 0, sizeof(head)); head.opcode = RX_CHANGED; head.flags = op->flags; head.count = op->count; @@ -560,6 +562,7 @@ static enum hrtimer_restart bcm_rx_timeout_handler(struct hrtimer *hrtimer) } /* create notification to user */ + memset(&msg_head, 0, sizeof(msg_head)); msg_head.opcode = RX_TIMEOUT; msg_head.flags = op->flags; msg_head.count = op->count;