linux-kernel - Re: [PATCH V3] greybus: gb-beagleplay: Ensure le for values in transport

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: <4cafbb5a-8ecd-407e-81a0-76d6505d013b@gmail.com>
Date:   Tue, 5 Dec 2023 17:15:33 +0530
From:   Ayush Singh <ayushdevel1325@...il.com>
To:     Greg KH <gregkh@...uxfoundation.org>
Cc:     Johan Hovold <johan@...nel.org>, greybus-dev@...ts.linaro.org,
        elder@...nel.org, linux-kernel@...r.kernel.org,
        jkridner@...gleboard.org, kernel test robot <yujie.liu@...el.com>
Subject: Re: [PATCH V3] greybus: gb-beagleplay: Ensure le for values in
 transport


On 12/5/23 05:44, Greg KH wrote:
> On Mon, Dec 04, 2023 at 09:58:55PM +0530, Ayush Singh wrote:
>> On 12/4/23 19:42, Johan Hovold wrote:
>>> On Mon, Dec 04, 2023 at 06:40:06PM +0530, Ayush Singh wrote:
>>>> Ensure that the following values are little-endian:
>>>> - header->pad (which is used for cport_id)
>>>> - header->size
>>>>
>>>> Fixes: ec558bbfea67 ("greybus: Add BeaglePlay Linux Driver")
>>>> Reported-by: kernel test robot <yujie.liu@...el.com>
>>>> Closes: https://lore.kernel.org/r/202311072329.Xogj7hGW-lkp@intel.com/
>>>> Signed-off-by: Ayush Singh <ayushdevel1325@...il.com>
>>>> ---
>>>> V3:
>>>> - Fix endiness while sending.
>>>> V2: https://lists.linaro.org/archives/list/greybus-dev@lists.linaro.org/thread/L53UN5ROSG4M6OE7CU5Y3L5F44T6ZPCC/
>>>> - Ensure endianess for header->pad
>>>> V1: https://lists.linaro.org/archives/list/greybus-dev@lists.linaro.org/message/K7UJ6PEAWBLNDMHLT2IO6OP5LQISHRUO/
>>>>
>>>>    drivers/greybus/gb-beagleplay.c | 9 +++++----
>>>>    1 file changed, 5 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/drivers/greybus/gb-beagleplay.c b/drivers/greybus/gb-beagleplay.c
>>>> index 43318c1993ba..8b21c3e1e612 100644
>>>> --- a/drivers/greybus/gb-beagleplay.c
>>>> +++ b/drivers/greybus/gb-beagleplay.c
>>>> @@ -93,9 +93,9 @@ static void hdlc_rx_greybus_frame(struct gb_beagleplay *bg, u8 *buf, u16 len)
>>>>    	memcpy(&cport_id, hdr->pad, sizeof(cport_id));
>>>>    	dev_dbg(&bg->sd->dev, "Greybus Operation %u type %X cport %u status %u received",
>>>> -		hdr->operation_id, hdr->type, cport_id, hdr->result);
>>>> +		hdr->operation_id, hdr->type, le16_to_cpu(cport_id), hdr->result);
>>>> -	greybus_data_rcvd(bg->gb_hd, cport_id, buf, len);
>>>> +	greybus_data_rcvd(bg->gb_hd, le16_to_cpu(cport_id), buf, len);
>>> This looks broken; a quick against mainline (and linux-next) check shows
>>> cport_id to be u16.
>>>
>>> I think you want get_unaligned_le16() or something instead of that
>>> memcpy() above.
>> Thanks, will do.
>>> But that just begs the question: why has this driver repurposed the pad
>>> bytes like this? The header still says that these shall be set to zero.
>> So, the reason is multiplexing. The original gbridge setup used to do this,
>> so I followed it when I moved gbridge to the coprocessor (during GSoC).
>>
>> Using the padding for storing cport information allows not having to wrap
>> the message in some other format at the two transport layers (UART and TCP
>> sockets) beagle connect is using.This also seems better than trying to do
>> something bespoke, especially since the padding bytes are not being used for
>> anything else.
>>
>> The initial spec was for project Ara (modular smartphone), so the current
>> use for IoT is significantly different from the initial goals of the
>> protocol. Maybe a future version of the spec can be more focused on IoT, but
>> that will probably only happen once it has proven somewhat useful in this
>> space.
> Please don't violate the spec today this way, I missed that previously,
> that's not ok.  We can change the spec for new things if you need it,
> but to not follow it, and still say it is "greybus" isn't going to work
> and will cause problems in the long-run.
>
> Should I just disable the driver for now until this is fixed up?
>
> thanks,
>
> greg k-h

Well, I will look into some ways to pass along the cport information 
(maybe using a wrapper over greybus message) for now. However, I would 
prefer having some bytes in greybus messages reserved for passing around 
this information in a transport agnostic way.

Ayush Singh