Re: [PATCH] mtd: require write permissions for locking and badblock ioctls
On Wed, 2021-03-03 at 15:57:35 UTC, Michael Walle wrote: > MEMLOCK, MEMUNLOCK and OTPLOCK modify protection bits. Thus require > write permission. Depending on the hardware MEMLOCK might even be > write-once, e.g. for SPI-NOR flashes with their WP# tied to GND. OTPLOCK > is always write-once. > > MEMSETBADBLOCK modifies the bad block table. > > Fixes: f7e6b19bc764 ("mtd: properly check all write ioctls for permissions") > Signed-off-by: Michael Walle > Reviewed-by: Greg Kroah-Hartman > Acked-by: Rafał Miłecki > Acked-by: Richard Weinberger Applied to https://git.kernel.org/pub/scm/linux/kernel/git/mtd/linux.git mtd/next, thanks. Miquel
Re: [PATCH] mtd: require write permissions for locking and badblock ioctls
- Ursprüngliche Mail - > Von: "Rafał Miłecki" > An: "Michael Walle" , "linux-mtd" > , "linux-kernel" > > CC: "Miquel Raynal" , "richard" , > "Vignesh Raghavendra" , > "Greg Kroah-Hartman" > Gesendet: Montag, 22. März 2021 17:39:41 > Betreff: Re: [PATCH] mtd: require write permissions for locking and badblock > ioctls > On 03.03.2021 16:57, Michael Walle wrote: >> MEMLOCK, MEMUNLOCK and OTPLOCK modify protection bits. Thus require >> write permission. Depending on the hardware MEMLOCK might even be >> write-once, e.g. for SPI-NOR flashes with their WP# tied to GND. OTPLOCK >> is always write-once. >> >> MEMSETBADBLOCK modifies the bad block table. >> >> Fixes: f7e6b19bc764 ("mtd: properly check all write ioctls for permissions") >> Signed-off-by: Michael Walle > > Should be fine for OpenWrt tools to my best knowledge (and quick testing). > > Acked-by: Rafał Miłecki Nice! Acked-by: Richard Weinberger Thanks, //richard
Re: [PATCH] mtd: require write permissions for locking and badblock ioctls
On 03.03.2021 16:57, Michael Walle wrote: MEMLOCK, MEMUNLOCK and OTPLOCK modify protection bits. Thus require write permission. Depending on the hardware MEMLOCK might even be write-once, e.g. for SPI-NOR flashes with their WP# tied to GND. OTPLOCK is always write-once. MEMSETBADBLOCK modifies the bad block table. Fixes: f7e6b19bc764 ("mtd: properly check all write ioctls for permissions") Signed-off-by: Michael Walle Should be fine for OpenWrt tools to my best knowledge (and quick testing). Acked-by: Rafał Miłecki
Re: [PATCH] mtd: require write permissions for locking and badblock ioctls
- Ursprüngliche Mail - >>> Thanks for auditing the rest of these from my original patch. If this >>> is ok with userspace tools, it's fine with me, but I don't even have >>> this hardware to test with :) >> >> That's my fear. Michael, did you verify? > > I don't know any tools except the mtd-utils. So no. openwrt folks have their own tooling, Anrdoid too. >> In general you need to be root to open these device files. >> So, I don't see a security problem here. > > Then this begs the question, why is this check there in > the first place? I fear historical reasons. As soon you can open the device node you can do evil things. > This come up because I was adding a OTPERASE which > was suggested that is was a "dangerous" command. So I > was puzzled why the ones above are considered "safe" ;) :-) Thanks, //richard
Re: [PATCH] mtd: require write permissions for locking and badblock ioctls
Am 2021-03-03 17:17, schrieb Richard Weinberger: Michael, - Ursprüngliche Mail - Von: "Greg Kroah-Hartman" An: "Michael Walle" CC: "linux-mtd" , "linux-kernel" , "Miquel Raynal" , "richard" , "Vignesh Raghavendra" Gesendet: Mittwoch, 3. März 2021 17:08:56 Betreff: Re: [PATCH] mtd: require write permissions for locking and badblock ioctls On Wed, Mar 03, 2021 at 04:57:35PM +0100, Michael Walle wrote: MEMLOCK, MEMUNLOCK and OTPLOCK modify protection bits. Thus require write permission. Depending on the hardware MEMLOCK might even be write-once, e.g. for SPI-NOR flashes with their WP# tied to GND. OTPLOCK is always write-once. MEMSETBADBLOCK modifies the bad block table. Fixes: f7e6b19bc764 ("mtd: properly check all write ioctls for permissions") Signed-off-by: Michael Walle --- drivers/mtd/mtdchar.c | 8 1 file changed, 4 insertions(+), 4 deletions(-) Thanks for auditing the rest of these from my original patch. If this is ok with userspace tools, it's fine with me, but I don't even have this hardware to test with :) That's my fear. Michael, did you verify? I don't know any tools except the mtd-utils. So no. In general you need to be root to open these device files. So, I don't see a security problem here. Then this begs the question, why is this check there in the first place? This come up because I was adding a OTPERASE which was suggested that is was a "dangerous" command. So I was puzzled why the ones above are considered "safe" ;) -michael
Re: [PATCH] mtd: require write permissions for locking and badblock ioctls
Michael, - Ursprüngliche Mail - > Von: "Greg Kroah-Hartman" > An: "Michael Walle" > CC: "linux-mtd" , "linux-kernel" > , "Miquel Raynal" > , "richard" , "Vignesh > Raghavendra" > Gesendet: Mittwoch, 3. März 2021 17:08:56 > Betreff: Re: [PATCH] mtd: require write permissions for locking and badblock > ioctls > On Wed, Mar 03, 2021 at 04:57:35PM +0100, Michael Walle wrote: >> MEMLOCK, MEMUNLOCK and OTPLOCK modify protection bits. Thus require >> write permission. Depending on the hardware MEMLOCK might even be >> write-once, e.g. for SPI-NOR flashes with their WP# tied to GND. OTPLOCK >> is always write-once. >> >> MEMSETBADBLOCK modifies the bad block table. >> >> Fixes: f7e6b19bc764 ("mtd: properly check all write ioctls for permissions") >> Signed-off-by: Michael Walle >> --- >> drivers/mtd/mtdchar.c | 8 >> 1 file changed, 4 insertions(+), 4 deletions(-) > > Thanks for auditing the rest of these from my original patch. If this > is ok with userspace tools, it's fine with me, but I don't even have > this hardware to test with :) That's my fear. Michael, did you verify? In general you need to be root to open these device files. So, I don't see a security problem here. Thanks, //richard
Re: [PATCH] mtd: require write permissions for locking and badblock ioctls
On Wed, Mar 03, 2021 at 04:57:35PM +0100, Michael Walle wrote: > MEMLOCK, MEMUNLOCK and OTPLOCK modify protection bits. Thus require > write permission. Depending on the hardware MEMLOCK might even be > write-once, e.g. for SPI-NOR flashes with their WP# tied to GND. OTPLOCK > is always write-once. > > MEMSETBADBLOCK modifies the bad block table. > > Fixes: f7e6b19bc764 ("mtd: properly check all write ioctls for permissions") > Signed-off-by: Michael Walle > --- > drivers/mtd/mtdchar.c | 8 > 1 file changed, 4 insertions(+), 4 deletions(-) Thanks for auditing the rest of these from my original patch. If this is ok with userspace tools, it's fine with me, but I don't even have this hardware to test with :) Reviewed-by: Greg Kroah-Hartman
[PATCH] mtd: require write permissions for locking and badblock ioctls
MEMLOCK, MEMUNLOCK and OTPLOCK modify protection bits. Thus require write permission. Depending on the hardware MEMLOCK might even be write-once, e.g. for SPI-NOR flashes with their WP# tied to GND. OTPLOCK is always write-once. MEMSETBADBLOCK modifies the bad block table. Fixes: f7e6b19bc764 ("mtd: properly check all write ioctls for permissions") Signed-off-by: Michael Walle --- drivers/mtd/mtdchar.c | 8 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/drivers/mtd/mtdchar.c b/drivers/mtd/mtdchar.c index 57c4a2f0b703..30c8273c1eff 100644 --- a/drivers/mtd/mtdchar.c +++ b/drivers/mtd/mtdchar.c @@ -643,16 +643,12 @@ static int mtdchar_ioctl(struct file *file, u_int cmd, u_long arg) case MEMGETINFO: case MEMREADOOB: case MEMREADOOB64: - case MEMLOCK: - case MEMUNLOCK: case MEMISLOCKED: case MEMGETOOBSEL: case MEMGETBADBLOCK: - case MEMSETBADBLOCK: case OTPSELECT: case OTPGETREGIONCOUNT: case OTPGETREGIONINFO: - case OTPLOCK: case ECCGETLAYOUT: case ECCGETSTATS: case MTDFILEMODE: @@ -663,9 +659,13 @@ static int mtdchar_ioctl(struct file *file, u_int cmd, u_long arg) /* "dangerous" commands */ case MEMERASE: case MEMERASE64: + case MEMLOCK: + case MEMUNLOCK: + case MEMSETBADBLOCK: case MEMWRITEOOB: case MEMWRITEOOB64: case MEMWRITE: + case OTPLOCK: if (!(file->f_mode & FMODE_WRITE)) return -EPERM; break; -- 2.20.1