Yeah, Probably this is the case. So when it is understood we need to think how to fix it :)
Best regards, Petro пт, 1 квіт. 2022 р. о 17:41 Alan Carvalho de Assis <[email protected]> пише: > Hmm, I think oflag equal 0 on Unix(so MacOS)/Linux works because of it: > > #define O_RDONLY 00000000 > > So, in fact on Linux/Mac when we are opening a file with oflag 0 we > are opening it for reading only. > > On NuttX the value 0 is not defined, O_RDONLY is 1: > > #define O_RDONLY (1 << 0) > > BR, > > Alan > > On 4/1/22, Petro Karashchenko <[email protected]> wrote: > > Hello, > > > > Here I'm talking not about driver registration permission, but more about > > the "oflag" parameter to "open()" call. > > > > I just tried a quick example on MAC > > > > #include <fcntl.h> > > #include <stdio.h> > > > > int main(void) > > { > > int fd = open("test.txt", 0); > > if (fd < 0) > > printf("A\n"); > > else > > printf("B\n"); > > > > return 0; > > } > > > > The "B" is printed if the file exists. If you know the system that will > run > > this sample code and will print "A", please let me know. > > > > Best regards, > > Petro > > > > пт, 1 квіт. 2022 р. о 16:27 Alan Carvalho de Assis <[email protected]> > > пише: > > > >> I think the device file shouldn't be created with permission 000. > >> > >> Look inside your Linux /dev all device files have RW permission for > >> root, some give only R for group and others. > >> > >> So, probably we need to fix the device register creation, not removing > >> the flag check. > >> > >> BR, > >> > >> Alan > >> > >> On 4/1/22, Xiang Xiao <[email protected]> wrote: > >> > It's better to check ioctl callback too since ioctl means the driver > >> > has > >> > the compatibility of read(i)and write(o). > >> > > >> > On Fri, Apr 1, 2022 at 9:15 PM Petro Karashchenko < > >> > [email protected]> wrote: > >> > > >> >> So Alan do you suggest to remove inode_checkflags? > >> >> > >> >> On Fri, Apr 1, 2022, 4:13 PM Alan Carvalho de Assis > >> >> <[email protected]> > >> >> wrote: > >> >> > >> >> > Hi Petro, > >> >> > > >> >> > I saw your PR #1117 but I think opening a device file with flag 0 > is > >> >> > not correct, please see the open man-pages: > >> >> > > >> >> > alan@dev:/tmp$ man 2 open > >> >> > > >> >> > The argument flags must include one of the following > access > >> >> > modes: O_RDONLY, O_WRONLY, or > >> >> > O_RDWR. These request opening the file read-only, > >> >> > write-only, > >> >> > or read/write, respectively. > >> >> > > >> >> > Also the opengroup say something similar: > >> >> > > >> >> > > https://pubs.opengroup.org/onlinepubs/9699919799/functions/open.html > >> >> > > >> >> > "Values for oflag are constructed by a bitwise-inclusive OR of > flags > >> >> > from the following list, defined in <fcntl.h>. Applications shall > >> >> > specify exactly one of the first five values (file access modes) > >> >> > below > >> >> > in the value of oflag:" > >> >> > > >> >> > The man pages uses "MUST", the OpenGroups uses "SHALL", but > >> >> > according > >> >> > to RFC2119 they are equivalents: > >> >> > > >> >> > https://www.ietf.org/rfc/rfc2119.txt > >> >> > > >> >> > BR, > >> >> > > >> >> > Alan > >> >> > > >> >> > On 4/1/22, Petro Karashchenko <[email protected]> > wrote: > >> >> > > Hi, > >> >> > > > >> >> > > I want to resume this thread again because after reexamined code > >> >> > carefully > >> >> > > I found that VFS layer has an API > >> >> > > > >> >> > > int inode_checkflags(FAR struct inode *inode, int oflags) > >> >> > > { > >> >> > > if (((oflags & O_RDOK) != 0 && !inode->u.i_ops->read) || > >> >> > > ((oflags & O_WROK) != 0 && !inode->u.i_ops->write)) > >> >> > > { > >> >> > > return -EACCES; > >> >> > > } > >> >> > > else > >> >> > > { > >> >> > > return OK; > >> >> > > } > >> >> > > } > >> >> > > > >> >> > > That checks if read and write handlers are available, so all our > >> >> > discussion > >> >> > > about R/W mode for IOCTL does not make any sense. We either need > >> >> > > to > >> >> > remove > >> >> > > this check or register VFS nodes with proper permissions and open > >> >> > > files > >> >> > > with correct flags. So if the driver does not have neither read > >> >> > > nor > >> >> write > >> >> > > handlers the "0000" mode should be used and "0" should be used > >> during > >> >> > > opening of a file. Or we need to remove "inode_checkflags()". > >> >> > > > >> >> > > Best regards, > >> >> > > Petro > >> >> > > > >> >> > > пт, 28 січ. 2022 р. о 15:11 Petro Karashchenko > >> >> > > <[email protected]> > >> >> > > пише: > >> >> > > > >> >> > >> I see. Thank you for the feedback. I will rework changes to get > >> back > >> >> > >> read permissions. > >> >> > >> > >> >> > >> Best regards, > >> >> > >> Petro > >> >> > >> > >> >> > >> пт, 28 січ. 2022 р. о 14:41 Alan Carvalho de Assis > >> >> > >> <[email protected] > >> >> > > >> >> > >> пише: > >> >> > >> > > >> >> > >> > Hi Petro, > >> >> > >> > > >> >> > >> > The read permission is needed even when you just want to open > a > >> >> file: > >> >> > >> > > >> >> > >> > $ vim noreadfile > >> >> > >> > > >> >> > >> > $ chmod 0000 noreadfile > >> >> > >> > > >> >> > >> > $ ls -l noreadfile > >> >> > >> > ---------- 1 user user 5 jan 28 09:24 noreadfile > >> >> > >> > > >> >> > >> > $ cat noreadfile > >> >> > >> > cat: noreadfile: Permission denied > >> >> > >> > > >> >> > >> > You can even try to create a C program just to open it, and it > >> >> > >> > will > >> >> > >> > fail. > >> >> > >> > > >> >> > >> > See the man page of open function: > >> >> > >> > > >> >> > >> > The argument flags *must* include one of the following > >> >> access > >> >> > >> > modes: O_RDONLY, O_WRONLY, or > >> >> > >> > O_RDWR. These request opening the file read-only, > >> >> write-only, > >> >> > >> > or read/write, respectively. > >> >> > >> > > >> >> > >> > This man page makes it clear you must include an access mode, > >> >> > >> > but > >> >> > >> > I > >> >> > >> > passed 0 to the access mode flag of open() and it was > accepted, > >> >> > >> > but > >> >> > >> > when the file has permission 0000 it returns -EPERM: "Failed > to > >> >> > >> > open > >> >> > >> > file: error -1" > >> >> > >> > > >> >> > >> > BR, > >> >> > >> > > >> >> > >> > Alan > >> >> > >> > > >> >> > >> > On 1/28/22, Petro Karashchenko <[email protected]> > >> >> wrote: > >> >> > >> > > Hello, > >> >> > >> > > > >> >> > >> > > Yes, but how does this relate to "0000" mode for > >> >> > "register_driver()"? > >> >> > >> > > Maybe you can describe some use case so it will become more > >> >> > >> > > clear? > >> >> > >> > > Currently ioctl works fine if driver is registered with > >> >> > >> > > "0000" > >> >> > >> permission > >> >> > >> > > mode. > >> >> > >> > > > >> >> > >> > > Best regards, > >> >> > >> > > Petro > >> >> > >> > > > >> >> > >> > > пт, 28 січ. 2022 р. о 11:39 Xiang Xiao > >> >> > >> > > <[email protected] > >> >> > > >> >> > >> пише: > >> >> > >> > >> > >> >> > >> > >> If we want to do the correct permission check, the ioctl > >> >> > >> > >> handler > >> >> > >> needs to > >> >> > >> > >> check R/W bit by itself based on how the ioctl is > >> >> > >> > >> implemented. > >> >> > >> > >> Or follow up how Linux encode the needed permission into > >> >> > >> > >> each > >> >> > IOCTL: > >> >> > >> > >> > >> >> > >> > >> >> > > >> >> > >> > https://github.com/torvalds/linux/blob/master/include/uapi/asm-generic/ioctl.h#L85-L91 > >> >> > >> > >> and let's VFS layer do the check for each driver. > >> >> > >> > >> > >> >> > >> > >> On Fri, Jan 28, 2022 at 5:14 PM Petro Karashchenko < > >> >> > >> > >> [email protected]> wrote: > >> >> > >> > >> > >> >> > >> > >> > Hello team, > >> >> > >> > >> > > >> >> > >> > >> > Recently I have noticed that there are many places in > code > >> >> where > >> >> > >> > >> > register_driver() is called with non-zero mode with file > >> >> > operation > >> >> > >> > >> > structures that have neither read nor write APIs > >> implemented. > >> >> For > >> >> > >> > >> > example "ret = register_driver(path, &opamp_fops, 0444, > >> >> > >> > >> > dev);" > >> >> > >> > >> > while > >> >> > >> > >> > opamp_fops has only "opamp_open", "opamp_close" and > >> >> "opamp_ioctl" > >> >> > >> > >> > implemented. I made a PR to fix it > >> >> > >> > >> > https://github.com/apache/incubator-nuttx/pull/5347 and > >> >> > >> > >> > change > >> >> > >> > >> > mode > >> >> > >> > >> > from "0444" to "0000", but want to ask if anyone sees any > >> >> > drawback > >> >> > >> in > >> >> > >> > >> > such an approach? Maybe I'm missing something? > >> >> > >> > >> > > >> >> > >> > >> > Best regards, > >> >> > >> > >> > Petro > >> >> > >> > >> > > >> >> > >> > > > >> >> > >> > >> >> > > > >> >> > > >> >> > >> > > >> > > >
