netdev - Re: [PATCH net v2] mctp i2c: handle NULL header address

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: <20241021115833.GG402847@kernel.org>
Date: Mon, 21 Oct 2024 12:58:33 +0100
From: Simon Horman <horms@...nel.org>
To: Matt Johnston <matt@...econstruct.com.au>
Cc: Jeremy Kerr <jk@...econstruct.com.au>,
	"David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	Andrew Lunn <andrew+netdev@...n.ch>, Wolfram Sang <wsa@...nel.org>,
	netdev@...r.kernel.org, linux-kernel@...r.kernel.org,
	stable@...r.kernel.org, Dung Cao <dung@...amperecomputing.com>
Subject: Re: [PATCH net v2] mctp i2c: handle NULL header address

On Mon, Oct 21, 2024 at 12:35:26PM +0800, Matt Johnston wrote:
> daddr can be NULL if there is no neighbour table entry present,
> in that case the tx packet should be dropped.
> 
> saddr will normally be set by MCTP core, but in case it is NULL it
> should be set to the device address.
> 
> Incorrect indent of the function arguments is also fixed.
> 
> Fixes: f5b8abf9fc3d ("mctp i2c: MCTP I2C binding driver")
> Cc: stable@...r.kernel.org
> Reported-by: Dung Cao <dung@...amperecomputing.com>
> Signed-off-by: Matt Johnston <matt@...econstruct.com.au>
> ---
> Changes in v2:
> - Set saddr to device address if NULL, mention in commit message
> - Fix patch prefix formatting
> - Link to v1: https://lore.kernel.org/r/20241018-mctp-i2c-null-dest-v1-1-ba1ab52966e9@codeconstruct.com.au
> ---
>  drivers/net/mctp/mctp-i2c.c | 13 ++++++++++---
>  1 file changed, 10 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/mctp/mctp-i2c.c b/drivers/net/mctp/mctp-i2c.c
> index 4dc057c121f5d0fb9c9c48bf16b6933ae2f7b2ac..c909254e03c21518c17daf8b813e610558e074c1 100644
> --- a/drivers/net/mctp/mctp-i2c.c
> +++ b/drivers/net/mctp/mctp-i2c.c
> @@ -579,7 +579,7 @@ static void mctp_i2c_flow_release(struct mctp_i2c_dev *midev)
>  
>  static int mctp_i2c_header_create(struct sk_buff *skb, struct net_device *dev,
>  				  unsigned short type, const void *daddr,
> -	   const void *saddr, unsigned int len)
> +				  const void *saddr, unsigned int len)
>  {
>  	struct mctp_i2c_hdr *hdr;
>  	struct mctp_hdr *mhdr;

Hi Matt,

I think you should drop this hunk.
While it's nice to clean things up, in the context of other work [1],
this isn't really appropriate as part of a fix for net.

[1] https://docs.kernel.org/process/maintainer-netdev.html#clean-up-patches

> @@ -588,8 +588,15 @@ static int mctp_i2c_header_create(struct sk_buff *skb, struct net_device *dev,
>  	if (len > MCTP_I2C_MAXMTU)
>  		return -EMSGSIZE;
>  
> -	lldst = *((u8 *)daddr);
> -	llsrc = *((u8 *)saddr);
> +	if (daddr)
> +		lldst = *((u8 *)daddr);
> +	else
> +		return -EINVAL;
> +
> +	if (saddr)
> +		llsrc = *((u8 *)saddr);
> +	else
> +		llsrc = dev->dev_addr;

This last line doesn't seem right, as llsrc is a u8,
while dev->dev_addr is a pointer to unsigned char.

>  
>  	skb_push(skb, sizeof(struct mctp_i2c_hdr));
>  	skb_reset_mac_header(skb);

-- 
pw-bot: changes-requested