linux-kernel - Re: [PATCH v7 4/4] tty: serial: 8250: add DFL bus driver for Altera 16550.

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: <alpine.DEB.2.22.394.2212211404270.570436@rhweight-WRK1>
Date:   Wed, 21 Dec 2022 14:16:38 -0800 (PST)
From:   matthew.gerlach@...ux.intel.com
To:     Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
cc:     hao.wu@...el.com, yilun.xu@...el.com, russell.h.weight@...el.com,
        basheer.ahmed.muddebihal@...el.com, trix@...hat.com,
        mdf@...nel.org, linux-fpga@...r.kernel.org,
        linux-doc@...r.kernel.org, linux-kernel@...r.kernel.org,
        tianfei.zhang@...el.com, corbet@....net,
        gregkh@...uxfoundation.org, linux-serial@...r.kernel.org,
        jirislaby@...nel.org, geert+renesas@...der.be,
        niklas.soderlund+renesas@...natech.se, macro@...am.me.uk,
        johan@...nel.org, lukas@...ner.de, ilpo.jarvinen@...ux.intel.com,
        marpagan@...hat.com, bagasdotme@...il.com
Subject: Re: [PATCH v7 4/4] tty: serial: 8250: add DFL bus driver for Altera
 16550.



On Tue, 20 Dec 2022, Andy Shevchenko wrote:

> On Tue, Dec 20, 2022 at 08:36:52AM -0800, matthew.gerlach@...ux.intel.com wrote:
>> From: Matthew Gerlach <matthew.gerlach@...ux.intel.com>
>>
>> Add a Device Feature List (DFL) bus driver for the Altera
>> 16550 implementation of UART.
>
> In general the code here looks good to me, but one thing to discuss due to
> comment to the previous patch(es).
>
> ...
>
>> +	u64 *p;
>> +
>> +	p = dfh_find_param(dfl_dev, DFHv1_PARAM_ID_CLK_FRQ);
>> +	if (!p)
>> +		return dev_err_probe(dev, -EINVAL, "missing CLK_FRQ param\n");
>> +
>> +	p++;
>> +	uart->port.uartclk = *p;
>
> So, here and the below is using always the second u64 from the returned data.
> Does it mean:
> - we always skip the first u64 from the returned buffer and hence... (see below)

The first u64 of the parameter block, the parameter header, contains a 
version field and a next/size field that a parameter consumer might use.
The version field determines the exact layout of the data, and the 
next/size field could/should be used to prevent out of bounds accesses.

> - we may actually return the second u64 as a plain number (not a pointer) from
>  (an additional?) API? In such case we would not need to take care about this
>  p++; lines here and there.

I think an additional API that can be used to fetch an array of u64's 
while also checking boundary conditions would be helpful.

> - we have fixed length of the data, returned by find_param(), i.e. 2 u64 words?

The length and layout of the parameter data is determined by the parameter 
id and version. So the data portion of a parameter is not fixed length.

Thanks for the feedback,
Matthew Gerlach

>
> -- 
> With Best Regards,
> Andy Shevchenko
>
>
>