Re: [PATCH] can: gw: validate checksum indices against frame length

[Oliver Hartkopp]

Hello PengPeng,

many thanks for your contribution!

On 30.06.26 09:29, Pengpeng Hou wrote:
> CAN gateway checksum parameters are validated against the maximum Classic
> CAN or CAN FD payload size when the route is configured.  At runtime,
> however, an individual frame may carry a shorter payload.

Right! I definitely get your point.

But the cf->len has to be seen as a magnifying glass when looking at the 
resulting CAN(FD) frame content.

With the modification rules AND,OR,XOR,SET always the entire(!) CAN 
frame content is modified - no matter what the cf->len is.
So it is possible that content is changed that is 'behind' cf->len.

Simple approach: As long as it fits to the CAN(FD) frame data structure 
the modification is applied.

Same applies to the XOR/CRC8 modification rules.

With your patch this simplicity would be removed without adding a new 
benefit. When the creator of the modification rule created an 
insufficient rule he will get an insufficient result.

With your patch the result will still be insufficient - but with 
different (insufficient) resulting content as you stop the processing at 
a different point.

As there is no qualitative improvement for the implemented functionality 
the patch finally would only add complexity, sorry.

Many thanks anyway and best regards,
Oliver

> The checksum helpers use from_idx, to_idx and result_idx to read and
> write cf->data[], and the CRC8 16U8 profile additionally reads
> cf->data[1].  A configuration that fits the maximum frame size can
> therefore still access bytes outside the current frame's valid payload.
>
> Validate checksum source and result indices against the current
> cf->len, and require byte 1 to be present before using the CRC8 16U8
> profile selector.
>
> Signed-off-by: Pengpeng Hou <pengpeng@xxxxxxxxxxx>
> ---
>   net/can/gw.c | 35 ++++++++++++++++++++++++++++++++++-
>   1 file changed, 34 insertions(+), 1 deletion(-)
>
> diff --git a/net/can/gw.c b/net/can/gw.c
> index 0ec99f6..5911afb 100644
> --- a/net/can/gw.c
> +++ b/net/can/gw.c
> @@ -318,6 +318,19 @@ static inline int calc_idx(int idx, int rx_len)
>   		return idx;
>   }
>   
> +static bool cgw_csum_idx_valid(const struct canfd_frame *cf, int idx)
> +{
> +	return idx >= 0 && idx < cf->len;
> +}
> +
> +static bool cgw_csum_range_valid(const struct canfd_frame *cf,
> +				 int from, int to, int res)
> +{
> +	return cgw_csum_idx_valid(cf, from) &&
> +	       cgw_csum_idx_valid(cf, to) &&
> +	       cgw_csum_idx_valid(cf, res);
> +}
> +
>   static void cgw_csum_xor_rel(struct canfd_frame *cf, struct cgw_csum_xor *xor)
>   {
>   	int from = calc_idx(xor->from_idx, cf->len);
> @@ -326,7 +339,7 @@ static void cgw_csum_xor_rel(struct canfd_frame *cf, struct cgw_csum_xor *xor)
>   	u8 val = xor->init_xor_val;
>   	int i;
>   
> -	if (from < 0 || to < 0 || res < 0)
> +	if (!cgw_csum_range_valid(cf, from, to, res))
>   		return;
>   
>   	if (from <= to) {
> @@ -345,6 +358,10 @@ static void cgw_csum_xor_pos(struct canfd_frame *cf, struct cgw_csum_xor *xor)
>   	u8 val = xor->init_xor_val;
>   	int i;
>   
> +	if (!cgw_csum_range_valid(cf, xor->from_idx, xor->to_idx,
> +				  xor->result_idx))
> +		return;
> +
>   	for (i = xor->from_idx; i <= xor->to_idx; i++)
>   		val ^= cf->data[i];
>   
> @@ -356,6 +373,10 @@ static void cgw_csum_xor_neg(struct canfd_frame *cf, struct cgw_csum_xor *xor)
>   	u8 val = xor->init_xor_val;
>   	int i;
>   
> +	if (!cgw_csum_range_valid(cf, xor->from_idx, xor->to_idx,
> +				  xor->result_idx))
> +		return;
> +
>   	for (i = xor->from_idx; i >= xor->to_idx; i--)
>   		val ^= cf->data[i];
>   
> @@ -371,7 +392,10 @@ static void cgw_csum_crc8_rel(struct canfd_frame *cf,
>   	u8 crc = crc8->init_crc_val;
>   	int i;
>   
> -	if (from < 0 || to < 0 || res < 0)
> +	if (!cgw_csum_range_valid(cf, from, to, res))
> +		return;
> +
> +	if (crc8->profile == CGW_CRC8PRF_16U8 && cf->len < 2)
>   		return;
>   
>   	if (from <= to) {
> @@ -406,6 +430,13 @@ static void cgw_csum_crc8_pos(struct canfd_frame *cf,
>   	u8 crc = crc8->init_crc_val;
>   	int i;
>   
> +	if (!cgw_csum_range_valid(cf, crc8->from_idx, crc8->to_idx,
> +				  crc8->result_idx))
> +		return;
> +
> +	if (crc8->profile == CGW_CRC8PRF_16U8 && cf->len < 2)
> +		return;
> +
>   	for (i = crc8->from_idx; i <= crc8->to_idx; i++)
>   		crc = crc8->crctab[crc ^ cf->data[i]];
>   
> @@ -433,6 +464,13 @@ static void cgw_csum_crc8_neg(struct canfd_frame *cf,
>   	u8 crc = crc8->init_crc_val;
>   	int i;
>   
> +	if (!cgw_csum_range_valid(cf, crc8->from_idx, crc8->to_idx,
> +				  crc8->result_idx))
> +		return;
> +
> +	if (crc8->profile == CGW_CRC8PRF_16U8 && cf->len < 2)
> +		return;
> +
>   	for (i = crc8->from_idx; i >= crc8->to_idx; i--)
>   		crc = crc8->crctab[crc ^ cf->data[i]];
>