Re: [PATCH net] net: ethernet: adi: adin1110: Fix some error handling path in adin1110_read_fifo()

[Christophe JAILLET]

Le 04/10/2024 à 13:37, Simon Horman a écrit :
> On Thu, Oct 03, 2024 at 08:53:15PM +0200, Christophe JAILLET wrote:
> > If 'frame_size' is too small or if 'round_len' is an error code, it is
> > likely that an error code should be returned to the caller.
> >
> > Actually, 'ret' is likely to be 0, so if one of these sanity checks fails,
> > 'success' is returned.
>
> Hi Christophe,
>
> I think we can say "'ret' will be 0".

Agreed.

	ret = adin1110_read_reg()
	--> spi_sync_transfer()
	--> spi_sync()

which explicitly documents "zero on success, else a negative error code."

> At least that is what my brief investigation tells me.
>
> > Return -EINVAL instead.

If the patch is considered as correct, can you confirm that -EINVAL is 
the correct error code to use? If not, which one would be preferred?


> Please include some information on how this was found and tested.
> e.g.
>
> Found by inspection / Found using widget-ng.

I would say: found by luck! :)

The explanation below will be of no help in the commit message and won't 
be added. I just give you all the gory details because you asked for it ;-)

(and after reading bellow, you can call me crazy!)



I was looking at functions that propagate error codes as their last 
argument. The idea came after submitting [1].

I read cci_read() and wondered if functions with such a semantic could 
use an un-initialized last argument. In such a case, this function could 
not behave as expected if the initial value of "err" was not 0.

So I wrote the following coccinelle script and several other variations.


// Options: --include-headers

@ok@
identifier fct, err;
type T;
@@

	int fct(..., T *err)
	{
		...
	}

@test depends on ok@
identifier x, fct = ok.fct;
expression res;
type T = ok.T;
@@

*	T x;
	...
(
	fct(..., &x);
|
	res = fct(..., &x);
)

(For the record, I have not found any issue with it...)


BUT, adin1110_read_fifo() was spotted because of the prototype of 
adin1110_read_reg().

When I reviewed the code, I quickly saw that it was a false positive and 
that using "type T" in my script was not that logical...

Anyway, when reviewing the code, I saw:

	if (ret < 0)
		return ret;

	/* The read frame size includes the extra 2 bytes
	 * from the  ADIN1110 frame header.
	 */
	if (frame_size < ADIN1110_FRAME_HEADER_LEN + ADIN1110_FEC_LEN)
		return ret;

	round_len = adin1110_round_len(frame_size);
	if (round_len < 0)
		return ret;

which looks really strange and likely broken...

Then I sent the patch we are talking about!


(yes some real people really search such things and write such 
coccinelle scripts, and now you can call me crazy)


[1]: 
https://lore.kernel.org/all/666ac169157f0af1c2e1d47926b68870cb39d587.1727977974.git.christophe.jaillet@xxxxxxxxxx/

> Compile tested only.

As a "speculative" patch, it was only compile tested, you are correct.

> > Fixes: bc93e19d088b ("net: ethernet: adi: Add ADIN1110 support")
> > Signed-off-by: Christophe JAILLET <christophe.jaillet@xxxxxxxxxx>
> > ---
> > This patch is speculative.
> > If returning 0 is what was intended, then an explicit 0 would be better.
>
> In my brief investigation I see that adin1110_read_fifo()
> is only called by adin1110_read_frames(), like this:
>
> 	while (budget) {
> 		...
>
> 		ret = adin1110_read_fifo(port_priv);
> 		if (ret < 0)
> 			return;
>
> 		budget--;
> 	}
>
> So the question becomes, should a failure in reading the fifo,
> because of an invalid frame size, be treated as an error
> and terminate reading frames.
>
> Like you, I speculate the answer is yes.
> But I think we need a bit more certainty to take this patch.

I won't be of any help here.

I can just say that "it looks strange" and is "certainly" bogus, but 
won't be able the prove it nor test it.


I'll wait a bit before sending a v2. If confirming this point is a 
requirement for accepting the patch, there is no need to urge for a v2 
if no-one cares about answering your point.

CJ


>