Hi,
On 9/9/24 2:06 PM, Andy Shevchenko wrote:
> On Mon, Sep 09, 2024 at 01:32:25PM +0200, Hans de Goede wrote:
>> The panasonic laptop code in various places uses the SINF array with index
>> values of 0 - SINF_CUR_BRIGHT(0x0d) without checking that the SINF array
>> is big enough.
>>
>> Not all panasonic laptops have this many SINF array entries, for example
>> the Toughbook CF-18 model only has 10 SINF array entries. So it only
>> supports the AC+DC brightness entries and mute.
>>
>> Check that the SINF array has a minimum size which covers all AC+DC
>> brightness entries and refuse to load if the SINF array is smaller.
>>
>> For higher SINF indexes hide the sysfs attributes when the SINF array
>> does not contain an entry for that attribute, avoiding show()/store()
>> accessing the array out of bounds and add bounds checking to the probe()
>> and resume() code accessing these.
>
> ...
>
>> +static umode_t pcc_sysfs_is_visible(struct kobject *kobj, struct attribute *attr, int idx)
>> +{
>> + struct device *dev = kobj_to_dev(kobj);
>> + struct acpi_device *acpi = to_acpi_device(dev);
>> + struct pcc_acpi *pcc = acpi_driver_data(acpi);
>
> Isn't it the same as dev_get_drvdata()?
No I also thought so and I checked. It is not the same,
struct acpi_device has its own driver_data member, which
this gets.
>
>> + if (attr == &dev_attr_mute.attr)
>> + return (pcc->num_sifr > SINF_MUTE) ? attr->mode : 0;
>> +
>> + if (attr == &dev_attr_eco_mode.attr)
>> + return (pcc->num_sifr > SINF_ECO_MODE) ? attr->mode : 0;
>> +
>> + if (attr == &dev_attr_current_brightness.attr)
>> + return (pcc->num_sifr > SINF_CUR_BRIGHT) ? attr->mode : 0;
>> +
>> + return attr->mode;
>> +}
>
> ...
>
>> + /*
>> + * pcc->sinf is expected to at least have the AC+DC brightness entries.
>> + * Accesses to higher SINF entries are checked against num_sifr.
>> + */
>> + if (num_sifr <= SINF_DC_CUR_BRIGHT || num_sifr > 255) {
>> + pr_err("num_sifr %d out of range %d - 255\n", num_sifr, SINF_DC_CUR_BRIGHT + 1);
>
> acpi_handle_err() ?
The driver is using pr_err() already in 18 other places, so IMHO it is better
to be consistent and also use it here.
Regards,
Hans
