Re: [PATCH v5 2/9] rtc: abx80x: fix error check after i2c_smbus_read in read_alarm()

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [Medium] Uninitialized Stack Memory Use on short I2C reads

Pre-existing issues:
- [Medium] Incomplete Bug Fix / Unintended Return Value breaking hardware alarm initialization
--

--- Patch [2]: [PATCH v5 2/9] rtc: abx80x: fix error check after i2c_smbus_read in read_alarm() ---
commit 111542521cef97450110e2dc7483bce8d88f644f
Author: Antoni Pokusinski <[email protected]>

    rtc: abx80x: fix error check after i2c_smbus_read in read_alarm()
    
    The patch relaxes the error check for i2c_smbus_read_i2c_block_data()
    in abx80x_read_alarm(). It now only returns on negative errors, allowing
    the function to proceed if the read is successful and returns the number
    of bytes read.

> diff --git a/drivers/rtc/rtc-abx80x.c b/drivers/rtc/rtc-abx80x.c
> index 5486d9d0b1e5d..fdf9664ebb6f0 100644
> --- a/drivers/rtc/rtc-abx80x.c
> +++ b/drivers/rtc/rtc-abx80x.c
> @@ -294,7 +294,7 @@ static int abx80x_read_alarm(struct device *dev, struct rtc_wkalrm *t)
>  
>  	err = i2c_smbus_read_i2c_block_data(client, ABX8XX_REG_ASC,
>  					    sizeof(buf), buf);
> -	if (err)
> +	if (err < 0)

[Severity: Medium]
Since the error check now only stops on negative values, could a short
read cause issues here?

If the adapter returns fewer bytes than sizeof(buf) (e.g. 1 to 6 bytes), err
will be positive but less than 7. The function will proceed to parse the
buffer, but since the stack allocated buf is uninitialized, wouldn't it end
up reading uninitialized stack memory later when checking buf[6]?

drivers/rtc/rtc-abx80x.c:abx80x_read_alarm() {
    ...
    t->pending = (buf[6] & ABX8XX_STATUS_AF) && t->enabled;
    ...
}

>  		return err;
>  
>  	irq_mask = i2c_smbus_read_byte_data(client, ABX8XX_REG_IRQ);

[Severity: Medium]
This is a pre-existing issue, but does returning err at the end of this
function cause problems with alarm initialization?

Since err holds the positive byte count on success, abx80x_read_alarm()
returns 7 instead of 0:

drivers/rtc/rtc-abx80x.c:abx80x_read_alarm() {
    ...
    err = i2c_smbus_read_i2c_block_data(...);
    ...
    return err;
}

Looking at how the rtc core handles this, __rtc_read_alarm() treats any
non-zero return as an error. Would this cause __devm_rtc_register_device()
to skip calling rtc_initialize_alarm() at boot?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2