netdev - Re: [PATCH net] mscan: zero accidentally copied register content

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAOCMmufQ77Vz4XTSTZTVSMiOA4QtWkZEvWHjGXip5-JK20E=QA@mail.gmail.com>
Date:	Thu, 6 Oct 2011 17:03:19 +0200
From:	Andre Naujoks <nautsch@...il.com>
To:	Oliver Hartkopp <socketcan@...tkopp.net>
Cc:	Wolfgang Grandegger <wg@...ndegger.com>,
	Wolfram Sang <w.sang@...gutronix.de>,
	Linux Netdev List <netdev@...r.kernel.org>
Subject: Re: [PATCH net] mscan: zero accidentally copied register content

2011/10/6 Oliver Hartkopp <socketcan@...tkopp.net>:
> On 10/06/11 11:09, Wolfgang Grandegger wrote:
>
>> On 10/06/2011 09:02 AM, Oliver Hartkopp wrote:
>>>
>>> I think if one would like to rework the 16bit register access (which is used
>>> in the rx path /and/ in the tx path also) this should go via net-next after
>>> some discussion and testing.
>>
>> Why do you want to change 16-bit accesses in general? They are faster
>> than two 8 bit accesses.
>>
>>> IMHO this fix is small and clear and especially not risky. I wonder if
>>> reworking the 16 bit register access is worth the effort?
>>
>> I would prefer:
>>
>>       if (!(frame->can_id & CAN_RTR_FLAG)) {
>>               void __iomem *data = &regs->rx.dsr1_0;
>>               u16 *payload = (u16 *)frame->data;
>>
>>               for (i = 0; i < frame->can_dlc / 2; i++) {
>>                       *payload++ = in_be16(data);
>>                       data += 2 + _MSCAN_RESERVED_DSR_SIZE;
>>               }
>>               /* copy remaining byte */
>>               if (frame->can_dlc & 1)
>>                       frame->data[frame->can_dlc - 1] = in_8(data);
>>       }
>
>
> Besides the fact that Andre is going to test this idea from Wolfgang now, are
> you really sure that it must be
>
>        in_8(data)
>
> and not
>
>        in_8(data+1)
>
> ???
>
> And that data definitely points to the right place?
>
> I would prefer to be really cautious with these big endian 16 bit registers!
>
> Therefore my fix with
>
> +               /* zero accidentally copied register content at odd DLCs */
> +               if (frame->can_dlc & 1)
> +                       frame->data[frame->can_dlc] = 0;
>
> only repairing the result looks much more defensive to me.

First things first: Both ways seem to work correctly. At least on the
MPC5200 I have here.

But I am with Oliver on this one. The solution looks much simpler and
endianess errors are not possible. If the few CPU cycles are worth it
on the other hand, then Wolfgangs version is probably preferable. I
don't have access to this kind of hardware on a little endian machine
to test it, though.

Greetings
  Andre

>
> Regards,
> Oliver
>
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html