linux-kernel - Re: [PATCH 04/10] phy: dphy: Add configuration helpers

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:   Mon, 10 Sep 2018 16:16:03 +0200
From:   Maxime Ripard <maxime.ripard@...tlin.com>
To:     Laurent Pinchart <laurent.pinchart@...asonboard.com>
Cc:     Kishon Vijay Abraham I <kishon@...com>,
        Boris Brezillon <boris.brezillon@...tlin.com>,
        Thomas Petazzoni <thomas.petazzoni@...tlin.com>,
        linux-media@...r.kernel.org,
        Archit Taneja <architt@...eaurora.org>,
        Andrzej Hajda <a.hajda@...sung.com>,
        Chen-Yu Tsai <wens@...e.org>, linux-kernel@...r.kernel.org,
        dri-devel@...ts.freedesktop.org,
        linux-arm-kernel@...ts.infradead.org,
        Krzysztof Witos <kwitos@...ence.com>,
        Rafal Ciepiela <rafalc@...ence.com>
Subject: Re: [PATCH 04/10] phy: dphy: Add configuration helpers

Hi,

On Fri, Sep 07, 2018 at 05:26:29PM +0300, Laurent Pinchart wrote:
> > >> + */
> > >> +int phy_mipi_dphy_get_default_config(unsigned long pixel_clock,
> > >> +				     unsigned int bpp,
> > >> +				     unsigned int lanes,
> > >> +				     struct phy_configure_opts_mipi_dphy *cfg)
> > >> +{
> > >> +	unsigned long hs_clk_rate;
> > >> +	unsigned long ui;
> > >> +
> > >> +	if (!cfg)
> > >> +		return -EINVAL;
> > > 
> > > Should we really expect cfg to be NULL ?
> > 
> > It avoids a kernel panic and it's not in a hot patch, so I'd say yes?
> 
> A few line below you divide by the lanes parameter without checking whether it 
> is equal to 0 first, which would also cause issues.

You say that like it would be a bad thing to test for this.

> I believe that invalid values in input parameters should only be handled 
> explicitly when considered acceptable for the caller to pass such values. In 
> this case a NULL cfg pointer is a bug in the caller, which would get noticed 
> during development if the kernel panics.

In the common case, yes. In the case where that pointer is actually
being lost by the caller somewhere down the line and you have to wait
for a while before it happens, then having the driver inoperant
instead of just having a panic seems like the right thing to do.

Maxime

-- 
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Download attachment "signature.asc" of type "application/pgp-signature" (834 bytes)