Re: [PATCH v3 2/3] misc: add driver for ST M24LR series RFID/NFC EEPROM chips

[Greg KH <gregkh@linuxfoundation.org>]

On Fri, Jun 06, 2025 at 02:24:56PM +0000, Abd-Alrhman Masalkhi wrote:
> >> +	if (unlikely(!is_power_of_2(reg_size) || reg_size > 8)) {
> >> +		dev_dbg(dev,
> >> +			"Invalid register size: must be a power of 2 and <= 8 bytes (%u)\n",
> >> +			reg_size);
> >> +		return -EIO;
> >
> > Not -EINVAL?  This isn't an I/O error.
> 
> The last if statement is primarily for debugging purposes. The reg_size
> value is specified internally by the driver (not user-controlled), so
> this check helps catch potential mistakes in the driver's sysfs entry
> definitions. That's why I used -EIO instead of -EINVAL, as it's not due
> to invalid user input but rather an internal misconfiguration.

But you just leaked your internal check to userspace in a potential
error code, so you must justify why userspace would ever see it and what
it should do with it :)

Just make it EINVAL please.  And if this is something that can't
actually happen, don't check it at all.

> >> +		for (i = 0; i < n_sss; i++) {
> >> +			char *name = devm_kasprintf(dev, GFP_KERNEL, "sss%d", i);
> >> +
> >> +			sss[i].reg_size = 1;
> >> +			sss[i].reg_addr = i;
> >> +			sss[i].attr.attr.name = name;
> >> +			sss[i].attr.attr.mode = 0600;
> >> +			sss[i].attr.show = m24lr_ctl_show;
> >> +			sss[i].attr.store = m24lr_ctl_store;
> >> +
> >> +			err = device_create_file(dev, &sss[i].attr);
> >
> > You just raced with userspace and lost. This is not how to do this,
> > please do not dynamically create attributes (hint, this should have
> > errored out as you didn't correctly initialize them), but also:
> 
> I didn't fully understand where the race condition comes from. Is
> the issue caused by calling device_create_file() from within the
> probe() function, or is it due to the fact that the attributes
> are being allocated dynamically rather than defined statically?

From calling device_create_file() at any point in time.  The driver core
should be doing that, not individual drivers.

thanks,

greg k-h