Hi Michal,
On 1/3/19 9:33 PM, Michal Kubecek wrote:
On Thu, Jan 03, 2019 at 08:31:51PM +0100, Oliver Hartkopp wrote:
Anyway your modification makes definitely sense, as it allows to process CAN
FD frames in struct canfd_frame as long as only data is modified that is
also available in a struct can_frame. AND - as a bonus - it should work for
stable 3.2 too, when CAN FD was not even introduced. Good idea!
If it's ok for you I would like to re-send the patch together with the CVE
number and would like to credit your suggestion in the text and with
"Suggested-by:".
OK
Thanks!
I have created a new patch that allows CAN FD frames to be handled
within the possibilities a struct can_frame offers (so especially with
can_id and can_dlc). As checksum calculation currently only supports
'Classic CAN' CAN FD frames are dropped when the can_dlc becomes > 8.
Two other hints about the upcoming patch:
1. I'm still using the test "if (cf->can_dlc > 8)" with a proper comment
as CAN_MAX_DLC and CAN_MAX_DLEN are not defined in Linux stable 3.2 - so
that we can apply the patch to all versions of gw.c
2. The counter gwj->deleted_frames is increased when dropping *invalid*
frames as this indicates a potential misconfiguration.
gwj->dropped_frames is only used for allocation problems like OOM.
As reported to security list, cgw_csum_xor_rel() with negative offset can
then rewrite e.g. frag_list pointer in skb_shared_info, crashing the
system. With unprivileged user namespaces, this can be exploited by any
regular user.
This is wrong! First there is no negative offset, as can_dlc is a u8 value.
Therefore you can try to hit content in the tail of the skb only.
I probably didn't use the right term. By "negative offset" I meant the
value of cgw_csum_xor::result_idx which, if negative, is interpreted as
an offset relative to can_dlc.
If result_idx is negative the position is calculated back from the
receive dlc - so to some index between 0 and dlc.
If the (invalid) value of modified
can_dlc is sufficiently large (larger then actual nskb->len), userspace
can enforce writing past packet data.
Yes. This hits the point.
See http://bugzilla.suse.com/show_bug.cgi?id=1120386 (comment 1) for an
example which can crash unfixed kernel by rewriting a pointer in skb
shared data which is later dereferenced when the skb is freed.
Second can-gw rules can only be configured by *root* and not by any regular
user - and finally it is definitely not namespace related.
So the user root can configure a can-gw rule to shoot into the skb tail to
kill the machine. I can imagine better things to do when I'm already root
Sorry for the noise, I misread the code (and commit 90f62cf30a78) so
that I thought netlink_ns_capable() is used in net/can/gw.c; now I see
that it's netlink_capable() so that global CAP_NET_ADMIN is required
rather than namespace one and the bug cannot be exploited by a regular
user.
No problem :-)
Thanks for going through the code that deep!
Best regards,
Oliver
