Re: drivers/firmware/thead,th1520-aon.c:174 th1520_aon_power_update() error: buffer overflow 'data' 2 <= 3

[Michal Wilczynski]

Hi,
On 3/25/26 13:13, Dan Carpenter wrote:
> tree:   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git master
> head:   bbeb83d3182abe0d245318e274e8531e5dd7a948
> commit: e4b3cbd840e565484d0ad8d260d27c057466ed17 firmware: thead: Add AON firmware protocol driver
> config: riscv-randconfig-r073-20260325 (https://download.01.org/0day-ci/archive/20260325/202603251917.9rxYz4kf-lkp@xxxxxxxxx/config)
> compiler: clang version 23.0.0git (https://protect2.fireeye.com/v1/url?k=d678e7b9-b7f3f28f-d6796cf6-74fe485fffe0-2c4534b9bc72cb13&q=1&e=1ce670d0-4eef-4bf7-a0c1-c4094e13228a&u=https%3A%2F%2Fgithub.com%2Fllvm%2Fllvm-project 054e11d1a17e5ba88bb1a8ef32fad3346e80b186)
> rustc: rustc 1.88.0 (6b00bc388 2025-06-23)
> smatch: v0.5.0-9004-gb810ac53
> 
> If you fix the issue in a separate patch/commit (i.e. not just a new version of
> the same patch/commit), kindly add following tags
> | Reported-by: kernel test robot <lkp@xxxxxxxxx>
> | Reported-by: Dan Carpenter <dan.carpenter@xxxxxxxxxx>
> | Closes: https://lore.kernel.org/r/202603251917.9rxYz4kf-lkp@xxxxxxxxx/
> 
> New smatch warnings:
> drivers/firmware/thead,th1520-aon.c:174 th1520_aon_power_update() error: buffer overflow 'data' 2 <= 3
> 
> vim +/data +174 drivers/firmware/thead,th1520-aon.c
> 
> e4b3cbd840e5654 Michal Wilczynski 2025-03-11  162  int th1520_aon_power_update(struct th1520_aon_chan *aon_chan, u16 rsrc,
> e4b3cbd840e5654 Michal Wilczynski 2025-03-11  163  			    bool power_on)
> e4b3cbd840e5654 Michal Wilczynski 2025-03-11  164  {
> e4b3cbd840e5654 Michal Wilczynski 2025-03-11  165  	struct th1520_aon_msg_req_set_resource_power_mode msg = {};
> e4b3cbd840e5654 Michal Wilczynski 2025-03-11  166  	struct th1520_aon_rpc_msg_hdr *hdr = &msg.hdr;
> e4b3cbd840e5654 Michal Wilczynski 2025-03-11  167  	int ret;
> e4b3cbd840e5654 Michal Wilczynski 2025-03-11  168  
> e4b3cbd840e5654 Michal Wilczynski 2025-03-11  169  	hdr->svc = TH1520_AON_RPC_SVC_PM;
> e4b3cbd840e5654 Michal Wilczynski 2025-03-11  170  	hdr->func = TH1520_AON_PM_FUNC_SET_RESOURCE_POWER_MODE;
> e4b3cbd840e5654 Michal Wilczynski 2025-03-11  171  	hdr->size = TH1520_AON_RPC_MSG_NUM;
> e4b3cbd840e5654 Michal Wilczynski 2025-03-11  172  
> e4b3cbd840e5654 Michal Wilczynski 2025-03-11  173  	RPC_SET_BE16(&msg.resource, 0, rsrc);
> e4b3cbd840e5654 Michal Wilczynski 2025-03-11 @174  	RPC_SET_BE16(&msg.resource, 2,
> e4b3cbd840e5654 Michal Wilczynski 2025-03-11  175  		     (power_on ? TH1520_AON_PM_PW_MODE_ON :
> e4b3cbd840e5654 Michal Wilczynski 2025-03-11  176  				 TH1520_AON_PM_PW_MODE_OFF));
> 
> This is deliberate, but I'm surprised the runtime hardening we have
> done doesn't complain that we're writing outside the &msg.resource
> struct member...  Maybe the RPC_SET_BE16() macro adds enough indirection
> to confuse the checker.
> 
> Why not just do?:
> 
> 	msg.mode = cpu_to_be16(power_on ?
> 			       TH1520_AON_PM_PW_MODE_ON :
> 			       TH1520_AON_PM_PW_MODE_OFF);

I've sent a patch last year to fix this [1]. Maybe it can be picked up ?

[1] - https://lore.kernel.org/all/20250403131051.1097423-1-m.wilczynski@xxxxxxxxxxx/

Best regards,
-- 
Michal Wilczynski <m.wilczynski@xxxxxxxxxxx>

> 
> e4b3cbd840e5654 Michal Wilczynski 2025-03-11  177  
> e4b3cbd840e5654 Michal Wilczynski 2025-03-11  178  	ret = th1520_aon_call_rpc(aon_chan, &msg);
> e4b3cbd840e5654 Michal Wilczynski 2025-03-11  179  	if (ret)
> e4b3cbd840e5654 Michal Wilczynski 2025-03-11  180  		dev_err(aon_chan->cl.dev, "failed to power %s resource %d ret %d\n",
> e4b3cbd840e5654 Michal Wilczynski 2025-03-11  181  			power_on ? "up" : "off", rsrc, ret);
> e4b3cbd840e5654 Michal Wilczynski 2025-03-11  182  
> e4b3cbd840e5654 Michal Wilczynski 2025-03-11  183  	return ret;
> e4b3cbd840e5654 Michal Wilczynski 2025-03-11  184  }
>