linux-kernel - Re: [PATCH 1/1] greybus: gb-beagleplay: Remove use of pad bytes

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]

Date:   Thu, 7 Dec 2023 22:33:54 +0530
From:   Ayush Singh <ayushdevel1325@...il.com>
To:     Greg KH <gregkh@...uxfoundation.org>
Cc:     greybus-dev@...ts.linaro.org, johan@...nel.org, elder@...nel.org,
        linux-kernel@...r.kernel.org, jkridner@...gleboard.org, nm@...com,
        yujie.liu@...el.com
Subject: Re: [PATCH 1/1] greybus: gb-beagleplay: Remove use of pad bytes

>> + *
>> + * @cport: cport id
>> + * @hdr: greybus operation header
>> + * @payload: greybus message payload
>> + */
>> +struct hdlc_greybus_frame {
>> +	__le16 cport;
>> +	struct gb_operation_msg_hdr hdr;
>> +	u8 payload[];
>> +} __packed;
>> +
>>   static void hdlc_rx_greybus_frame(struct gb_beagleplay *bg, u8 *buf, u16 len)
>>   {
>> -	u16 cport_id;
>> -	struct gb_operation_msg_hdr *hdr = (struct gb_operation_msg_hdr *)buf;
>> +	struct hdlc_greybus_frame *gb_frame = (struct hdlc_greybus_frame *)buf;
>> +	u16 cport_id = le16_to_cpu(gb_frame->cport);
>>   
>> -	memcpy(&cport_id, hdr->pad, sizeof(cport_id));
>> +	/* Ensure that the greybus message is valid */
>> +	if (le16_to_cpu(gb_frame->hdr.size) > len - sizeof(cport_id)) {
>> +		dev_warn_ratelimited(&bg->sd->dev, "Invalid/Incomplete greybus message");
> Don't spam the kernel log for corrupted data on the line, that would be
> a mess.  Use a tracepoint?
>
>> +		return;
>> +	}
>>   
>>   	dev_dbg(&bg->sd->dev, "Greybus Operation %u type %X cport %u status %u received",
>> -		hdr->operation_id, hdr->type, le16_to_cpu(cport_id), hdr->result);
>> +		gb_frame->hdr.operation_id, gb_frame->hdr.type, cport_id, gb_frame->hdr.result);
> Better yet, put the error in the debug message?
Shouldn't corrupt data be a warning rather than debug message, since it 
indicates something wrong with the transport?
>>   
>> -	greybus_data_rcvd(bg->gb_hd, le16_to_cpu(cport_id), buf, len);
>> +	greybus_data_rcvd(bg->gb_hd, cport_id, &buf[sizeof(cport_id)],
> Fun with pointer math.  This feels really fragile, why not just point to
> the field instead?
It seems that taking address of members of packed structures is not 
valid. I get the `address-of-packed-member` warnings. Is it fine to 
ignore those in kernel?
>>   }
>>   
>>   static void hdlc_rx_dbg_frame(const struct gb_beagleplay *bg, const char *buf, u16 len)
>> @@ -339,7 +357,7 @@ static struct serdev_device_ops gb_beagleplay_ops = {
>>   static int gb_message_send(struct gb_host_device *hd, u16 cport, struct gb_message *msg, gfp_t mask)
>>   {
>>   	struct gb_beagleplay *bg = dev_get_drvdata(&hd->dev);
>> -	struct hdlc_payload payloads[2];
>> +	struct hdlc_payload payloads[3];
> why 3?
>
> It's ok to put this on the stack?

Well, the HDLC payload is just to store the length of the payload along 
with a pointer to its data. (kind of emulate a fat pointer). The reason 
for doing it this way is to avoid having to create a temp buffer for 
each message when sending data over UART (which was done in the initial 
version of the driver).

Ayush Singh