netdev - Re: [PATCH v2] ptp: ocp: Limit SMA/signal/freq counts in show/store functions

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAMuE1bHL1-t_YD0B5v1LuY_b558U5qbseSYJXvnm734+Vb-v_w@mail.gmail.com>
Date: Sun, 11 May 2025 17:39:08 +0300
From: Sagi Maimon <maimon.sagi@...il.com>
To: Jakub Kicinski <kuba@...nel.org>
Cc: jonathan.lemon@...il.com, vadim.fedorenko@...ux.dev, 
	richardcochran@...il.com, andrew+netdev@...n.ch, davem@...emloft.net, 
	edumazet@...gle.com, pabeni@...hat.com, linux-kernel@...r.kernel.org, 
	netdev@...r.kernel.org
Subject: Re: [PATCH v2] ptp: ocp: Limit SMA/signal/freq counts in show/store functions

On Sun, May 11, 2025 at 12:03 PM Sagi Maimon <maimon.sagi@...il.com> wrote:
>
> On Sun, May 11, 2025 at 11:16 AM Sagi Maimon <maimon.sagi@...il.com> wrote:
> >
> > On Sat, May 10, 2025 at 1:01 AM Jakub Kicinski <kuba@...nel.org> wrote:
> > >
> > > On Thu,  8 May 2025 10:19:01 +0300 Sagi Maimon wrote:
> > > > The sysfs show/store operations could access uninitialized elements in
> > > > the freq_in[], signal_out[], and sma[] arrays, leading to NULL pointer
> > > > dereferences. This patch introduces u8 fields (nr_freq_in, nr_signal_out,
> > > > nr_sma) to track the actual number of initialized elements, capping the
> > > > maximum at 4 for each array. The affected show/store functions are updated to
> > >
> > > This line is too long. I think the recommended limit for commit message
> > > is / was 72 or 74 chars.
> > >
> > will be fixed on next patch
> > > > respect these limits, preventing out-of-bounds access and ensuring safe
> > > > array handling.
> > >
> > > What do you mean by out-of-bounds access here. Is there any access with
> > > index > 4 possible? Or just with index > 1 for Adva?
> > >
The sysfs interface restricts indices to a maximum of 4; however,
since an array of 4 signals/frequencies is always created and fully
accessible via sysfs—regardless of the actual number initialized—this
bug impacts any board that initializes fewer than 4
signals/frequencies.
> > > We need more precise information about the problem to decide if this is
> > > a fix or an improvement
> > >
> > > > +     bp->sma_nr  = 4;
> > >
> > > nit: double space in all the sma_nr assignments
> > >
> > will be fixed on next patch
> > > >
> > > >       ptp_ocp_fb_set_version(bp);
> > > >
> > > > @@ -2862,6 +2870,9 @@ ptp_ocp_art_board_init(struct ptp_ocp *bp, struct ocp_resource *r)
> > > >       bp->fw_version = ioread32(&bp->reg->version);
> > > >       bp->fw_tag = 2;
> > > >       bp->sma_op = &ocp_art_sma_op;
> > > > +     bp->signals_nr = 4;
> > > > +     bp->freq_in_nr = 4;
> > > > +     bp->sma_nr  = 4;
> > > >
> > > >       /* Enable MAC serial port during initialisation */
> > > >       iowrite32(1, &bp->board_config->mro50_serial_activate);
> > > > @@ -2888,6 +2899,9 @@ ptp_ocp_adva_board_init(struct ptp_ocp *bp, struct ocp_resource *r)
> > > >       bp->flash_start = 0xA00000;
> > > >       bp->eeprom_map = fb_eeprom_map;
> > > >       bp->sma_op = &ocp_adva_sma_op;
> > > > +     bp->signals_nr = 2;
> > > > +     bp->freq_in_nr = 2;
> > > > +     bp->sma_nr  = 2;
> > > >
> > > >       version = ioread32(&bp->image->version);
> > > >       /* if lower 16 bits are empty, this is the fw loader. */
> > > > @@ -3002,6 +3016,9 @@ ptp_ocp_sma_show(struct ptp_ocp *bp, int sma_nr, char *buf,
> > > >       const struct ocp_selector * const *tbl;
> > > >       u32 val;
> > > >
> > > > +     if (sma_nr > bp->sma_nr)
> > > > +             return 0;
> > >
> > > Why are you returning 0 and not an error?
> > >
> > will be fixed next patch
> > > As a matter of fact why register the sysfs files for things which don't
> > > exists?
> > > --
> > The number of SMAs initialized via sysfs is shared across all boards,
> > necessitating a modification to this mechanism. Additionally, only the
> > freq_in[] and signal_out[] arrays are causing NULL pointer
> > dereferences. To address these issues, I will submit two separate
> > patches: one to handle the NULL pointer dereferences in signals and
> > freq_in, and another to refactor the SMA initialization process.
> >
> > > pw-bot: cr