linux-kernel - RE: [PATCH] tty: Add MOXA NPort Real TTY Driver

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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <HK2PR01MB32817A21FEDDC410F2822640FA790@HK2PR01MB3281.apcprd01.prod.exchangelabs.com>
Date:   Wed, 22 Jul 2020 07:04:00 +0000
From:   Johnson CH Chen (陳昭勳) 
        <JohnsonCH.Chen@...a.com>
To:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>
CC:     Jiri Slaby <jirislaby@...il.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-serial@...r.kernel.org" <linux-serial@...r.kernel.org>,
        Victor Yu (游勝義) <victor.yu@...a.com>,
        Danny Lin (林政易) <danny.lin@...a.com>
Subject: RE: [PATCH] tty: Add MOXA NPort Real TTY Driver

Hi Greg,

Thanks for your response!

> > > > +	unsigned long flag;
> > > > +	unsigned char cmd_buffer[84];
> > > > +	unsigned char rsp_buffer[84];
> > >
> > > You seem to have two "static" buffers here, for your device, that 
> > > you semi-randomly write to all over the place, but I can't find 
> > > any locking or coordination between things that prevents multiple 
> > > commands from not just overwritting each other.
> > >
> > For cmd_buffer[], we use npreal_wait_and_set_command() to make sure 
> > cmd_buffer[] is safe to be written by checking "cmd_buffer[0] == 0".
> 
> And what locks are protecting you there?
> 
> > For rsp_buffer[], we use npreal_wait_command_completed() to make 
> > sure rsp_buffer[] is desired by checking rsp_buffer[0] and rsp_buffer[1].
> > Command_set and command should be checked. Besides, rsp_buffer[] is 
> > got from user space by "NPREAL_NET_CMD_RESPONSE" in 
> > npreal_net_ioctl().
> 
> Again, what locking is really handling this?
> 

It's better to protect cmd_buffer[84] and rsp_buffer[84] by locking completely. They are safe because NPort driver should be worked with NPort daemon before, and NPort daemon is designed to be simple.

> > > Also, how does the data get sent to the hardware at all?  I see 
> > > cmd_buffer[] being written to, but what reads from it and how does 
> > > the hardware get the data?
> >
> > Actually we need to both NPort driver (this driver) and Npreal 
> > daemon
> > (userspace) to let HW work. Npreal daemon can communicate with HW by 
> > socket, and Npreal deamon communicates with Nport driver by 
> > "npreal_net_fops". When commands are ready for driver part, it will 
> > wake up poll event to let Nport daemon know.
> 
> That is not obvious at all, and needs to be really really really documented here.
> Why not put the userspace chunk in the tree too?  At the least, you 
> need to point at it.
> 
> And why is a userspace part needed?  We have tty-over-ethernet drivers 
> that do not require such a thing in the tree somewhere...
>

Because we need hardware serial to Ethernet converter (NPort device server) to manage some serial devices, so we still need to use MOXA Nport's commands and responses between host computer and converter. We will have an internal discussion about release of Nport daemon and related document, or using free tty to Ethernet daemon such as (ser2net/ socat/ remtty) and improved nport driver later. Thanks a lot!

Best regards,
Johnson